Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed change to Cro::HTTP::CookieJar #39

Merged
merged 14 commits into from May 22, 2019
Merged

Conversation

Xliff
Copy link
Contributor

@Xliff Xliff commented Jul 3, 2018

This PR will add the ability to add Cro::HTTP::Cookies directly to any Cro::HTTP::CookieJar object.

Copy link
Member

@jnthn jnthn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small things to change.

lib/Cro/HTTP/Client/CookieJar.pm6 Outdated Show resolved Hide resolved
lib/Cro/HTTP/Client/CookieJar.pm6 Outdated Show resolved Hide resolved
@Xliff
Copy link
Contributor Author

Xliff commented Jul 4, 2018

@jnthn:

Line 110:    @!cookies.push: $state.clone(cookie => $_.clone(:$domain, :$path));

Here is where I got the idea of cloning the state, because this is done in add-from-response. This is also done on a newly created CookieState object. I just wanted to mention it, since you brought it up.

@dolmen
Copy link

dolmen commented May 13, 2019

@jnthn Is there anything blocking this addition of the add-cookie method?

@jnthn
Copy link
Member

jnthn commented May 13, 2019

@dolmen Forgot about this one; taking a quick look now, I think:

  • There should be a --> Nil on the add-cookie method
  • It needs tests

@Xliff
Copy link
Contributor Author

Xliff commented May 13, 2019

Things for merge:

  • --> Nil
  • tests

Got it!

@Xliff
Copy link
Contributor Author

Xliff commented May 13, 2019

This last commit should cover what's missing. Please review.

t/http-cookiejar.t Outdated Show resolved Hide resolved
t/http-cookiejar.t Outdated Show resolved Hide resolved
@Xliff
Copy link
Contributor Author

Xliff commented May 16, 2019

Latest changes per jnthn's comments have been added.

@Xliff
Copy link
Contributor Author

Xliff commented May 22, 2019

This last bit should clean up the indentation issues. Please review.

1 similar comment
@Xliff
Copy link
Contributor Author

Xliff commented May 22, 2019

This last bit should clean up the indentation issues. Please review.

@jnthn jnthn merged commit 79d5469 into croservices:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants