-
-
Notifications
You must be signed in to change notification settings - Fork 76
refactor the CookieJar interface for more implementation independence #56
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
base: main
Are you sure you want to change the base?
refactor the CookieJar interface for more implementation independence #56
Conversation
AlexV525
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leptopoda Thanks for your contribution, and of course they are welcomed!
Regarding the breaking part, a detailed migration guide would be helpful for the process. Also you may want to check the compatibility with dio_cookie_manager and other downstream packages to learn their cases.
About RFCs, it would be great if you could ref them as comments directly in the code, so we can always follow them.
93416bc to
b952da4
Compare
|
Sorry it has been a while since the last review. Could you fix all the tests? |
|
The linting is failing, not the tests |
|
Yeah and the lints too |
Signed-off-by: Nikolas Rimikis <[email protected]>
…plementation Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
…ies as defined by RFC6265 Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
b952da4 to
08b66c4
Compare
|
Sorry for my inactivity. The CI should be passing now. |
|
Hi @Leptopoda . The last notification must be missed from my list. Do you have further changes remaining for the request? I shall review the request again soon. |
Hi there and thank you for this package.
I hereby suggest a few changes to the interface of the CookieJar class. We wanted to implement a custom cookie storage backend in sql and ended up writing a custom CookieJar instead of just using a custom
Storage.We did observe that the api is quite biased towards the
DefaultCookieJarimplementation and not really considering other implementations.We decided to go with:
FutureOrignoreExpiresthat not every cookie store needs (we strive for RFC compliance so we do not need the option to ignore the expiration).deletewith adeleteWherefunction so users of the api can decide with higher precision what cookies they want to be removed.endSessionfunction to remove non persistent cookies as described in the RFCThis is just a proposal and we are open to talk about changes. We hope you consider these changes even if they are breaking. Obviously there are less breaking ways to achieve this like keeping around the
deletefunction and just deprecating it for now.We are also open to upstream the rest of our RFC compliance focused code if you are open and interested in further changes.