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

fix typing on set_cookie #3836

Closed

Conversation

FlorianLudwig
Copy link

@FlorianLudwig FlorianLudwig commented Jun 11, 2019

What do these changes do?

Change type hints on set_cookie to match the existing documentation

Are there changes in behavior for the user?

For users that use linting, yes.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@FlorianLudwig FlorianLudwig requested a review from asvetlov as a code owner June 11, 2019 13:17
@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #3836 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3836      +/-   ##
==========================================
- Coverage   97.96%   97.95%   -0.01%     
==========================================
  Files          43       43              
  Lines        8584    11430    +2846     
  Branches     1372     2256     +884     
==========================================
+ Hits         8409    11196    +2787     
- Misses         71      135      +64     
+ Partials      104       99       -5
Impacted Files Coverage Δ
aiohttp/web_response.py 98.65% <100%> (+0.92%) ⬆️
aiohttp/worker.py 73.5% <0%> (-24.04%) ⬇️
aiohttp/resolver.py 86.9% <0%> (-13.1%) ⬇️
aiohttp/__init__.py 98.98% <0%> (-1.02%) ⬇️
aiohttp/web_runner.py 96.96% <0%> (-0.96%) ⬇️
aiohttp/client_proto.py 96% <0%> (-0.56%) ⬇️
aiohttp/test_utils.py 99.54% <0%> (-0.15%) ⬇️
aiohttp/http_parser.py 97.82% <0%> (-0.11%) ⬇️
aiohttp/web_exceptions.py 100% <0%> (ø) ⬆️
aiohttp/web_middlewares.py 100% <0%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b80fec6...6afef0f. Read the comment docs.

@asvetlov
Copy link
Member

Please fix CI, mypy linter is failed

@FlorianLudwig
Copy link
Author

in my humble opinion that is a bug in typeshed and opened it there: python/typeshed#3059

@asvetlov
Copy link
Member

Thank you for the PR.
I think we should wait for upstream fix first

@webknjaz webknjaz closed this Jul 29, 2019
@webknjaz webknjaz reopened this Jul 29, 2019
@FlorianLudwig
Copy link
Author

The upstream bug is fixed in MyPy 0.720 as it pulls in typeched f5c107cacd7c17ef541278bf228ca411fdeaa11b

@webknjaz
Copy link
Member

@FlorianLudwig the checks should pass before merging.

@FlorianLudwig FlorianLudwig requested a review from webknjaz as a code owner July 30, 2019 08:27
@FlorianLudwig
Copy link
Author

@webknjaz Bumping mypy to 0.720 is needed - which results in other new linting errors (see travis).

I can check those later, maybe over the weekend

@asvetlov
Copy link
Member

I can check those later, maybe over the weekend

Please do

@@ -170,8 +170,8 @@ def set_cookie(self, name: str, value: str, *,
max_age: Optional[Union[int, str]]=None,
path: str='/',
secure: Optional[str]=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about secure?

httponly: Optional[str]=None,
version: Optional[str]=None) -> None:
httponly: Optional[bool]=None,
version: Optional[bool]=None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is version really a boolean?

@asvetlov
Copy link
Member

@FlorianLudwig would you sync the PR with the latest master?
It uses mypy 0.740 now.

@serhiy-storchaka
Copy link
Contributor

And, if the current tests are passed with updated MyPy, would be nice to add tests for set_cookie() with these parameters.

@asvetlov
Copy link
Member

The problem is fixed by e4573f8 and 1220613

@asvetlov asvetlov closed this Jan 27, 2020
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.

5 participants