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

lifecycle: add cookies attribute to ConnexionRequest (#1168) #1209

Conversation

noirbee
Copy link
Contributor

@noirbee noirbee commented Apr 8, 2020

Fixes #1168.

Changes proposed in this pull request:

  • Add a cookies attribute to lifecycle.ConnexionRequest
  • Use flask.Request.cookies and aiohttp.web.Request.cookies

@coveralls
Copy link

coveralls commented Apr 8, 2020

Coverage Status

Coverage increased (+0.001%) to 96.877% when pulling 527ce20 on noirbee:fix/connexionrequest-cookies-attribute-1168 into 8830d56 on zalando:master.

@hjacobs
Copy link
Contributor

hjacobs commented Apr 25, 2020

Can you add unit tests?

@Sebastiencreoff
Copy link

It looks to be only managed for Flask, is it also possible to make it for aiohttp.
As I also need this change, how can I participate to it to have this feature ?

@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from 417ffb6 to 98e71e3 Compare May 15, 2020 15:34
@noirbee
Copy link
Contributor Author

noirbee commented May 15, 2020

Can you add unit tests?

@hjacobs I rebased the commit on master + added unit tests in tests/api/test_parameters.py

It looks to be only managed for Flask, is it also possible to make it for aiohttp.
As I also need this change, how can I participate to it to have this feature ?

@Sebastiencreoff I don't know enough about aiohttp to help here, could you test it with an actual application ? AFAICT the unit tests in tests/api/ only cover Flask app. I looked at the server API reference and it's basically the same as Flask (a MultiDict) so I expect the behaviour to be identical.

@dtkav
Copy link
Collaborator

dtkav commented Jul 29, 2020

FYI: aiohttp tests are in tests/aiohttp

@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from 98e71e3 to 527ce20 Compare July 29, 2020 10:56
@noirbee
Copy link
Contributor Author

noirbee commented Jul 29, 2020

Rebased on master, added an unit test for aiohttp.

@noirbee
Copy link
Contributor Author

noirbee commented Aug 12, 2020

Rebased on master, added an unit test for aiohttp.

@hjacobs is something else needed to merge this ?

@joe42
Copy link

joe42 commented Jan 4, 2021

Thanks for the bugfix!
Is there anything I can help with to get this merged?

@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from 527ce20 to e476552 Compare January 4, 2021 09:40
@HellDryx
Copy link

I'm having troubles with the issue this PR is fixing. Is there anything left preventing it from being merged ?

@joe42
Copy link

joe42 commented Feb 24, 2021

@HellDryx It seems that the repository has become inactive ;(

@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from e476552 to 9960d2e Compare August 6, 2021 14:05
@noirbee noirbee changed the base branch from master to main August 6, 2021 14:12
@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from 9960d2e to ff0fbee Compare August 6, 2021 14:17
@noirbee
Copy link
Contributor Author

noirbee commented Aug 6, 2021

@rafaelcaricio @Ruwann @RobbeSneyders sorry to ping you like this but I don't know how to ask for a new review / approval to merge.

I've updated the PR to use async instead of @asyncio.coroutine in the aiohttp tests, and switched the base to main.

@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from ff0fbee to a577307 Compare November 5, 2021 07:39
@noirbee noirbee force-pushed the fix/connexionrequest-cookies-attribute-1168 branch from a577307 to 2a9f405 Compare February 14, 2022 17:59
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1889829072

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 96.748%

Totals Coverage Status
Change from base Build 1883321425: 0.003%
Covered Lines: 2886
Relevant Lines: 2983

💛 - Coveralls

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thx @noirbee!

@RobbeSneyders RobbeSneyders merged commit a1dddf6 into spec-first:main Feb 23, 2022
@noirbee noirbee deleted the fix/connexionrequest-cookies-attribute-1168 branch February 24, 2022 08:19
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.

validate_cookie_parameter is using a non-existing ConnexionRequest attribute
8 participants