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

s_maxage converts to an int #2365

Merged
merged 1 commit into from
Apr 1, 2022
Merged

Conversation

Yourun-proger
Copy link
Contributor

@Yourun-proger Yourun-proger commented Mar 30, 2022

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

Sorry, something went wrong.

@davidism
Copy link
Member

This only addresses the specific attribute, there are others that have the same issue where type is set to None, and can be addressed at a higher level.

@Yourun-proger
Copy link
Contributor Author

@davidism, thanks for reasonable remark! Indeed, it is better to do everything at once, and not to correct identical issues one by one.
I just looked and found such problems here:

  • in ResponseCacheControl: field private (should be bool)
  • in _CacheControl class: fields no_transform (should be bool) and no_cache (should also be bool)

There is nothing like it anywhere else. Do I understand correctly that tests should also be written for all this?

@Yourun-proger
Copy link
Contributor Author

Oh. I've made all the necessary changes, but I'm not passing tests at this location:

         assert c.no_cache is None
         assert c.private is None

Because now these fields are by default casted to bool. And as you know, bool(None) is False.
Here's what should I do? Change these tests?

@davidism
Copy link
Member

davidism commented Mar 31, 2022

I'll figure out a way to deprecate the old behavior and transition to keys returning False or None if not set.

@Yourun-proger
Copy link
Contributor Author

I deleted that comment, but still I didn't get it. Do I need to do anything with failing tests?

@davidism
Copy link
Member

davidism commented Apr 1, 2022

No, you're fine.

@Yourun-proger
Copy link
Contributor Author

OK. Thanks a lot! I have been trying to implement this myself for the last couple of hours, but there are too many pitfalls in this case ...
I'll post the changes as soon as possible :3

@Yourun-proger Yourun-proger changed the title Fix bug in ResponseCacheControl.s_maxage behavior Fix bug in classes for cache Apr 1, 2022
@Yourun-proger
Copy link
Contributor Author

Yourun-proger commented Apr 1, 2022

Wow! I did not understand why git thinks that I changed so much in the docstrings, I just added info about 2.1.1

@davidism davidism changed the title Fix bug in classes for cache s_maxage converts to an int Apr 1, 2022
@davidism davidism added this to the 2.1.1 milestone Apr 1, 2022

Verified

This commit was signed with the committer’s verified signature.
davidism David Lord
@davidism davidism merged commit ae18aca into pallets:2.1.x Apr 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants