-
Notifications
You must be signed in to change notification settings - Fork 83
Jw/snow 2131951 less freqent update messages #2352
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
Jw/snow 2131951 less freqent update messages #2352
Conversation
05ba6aa to
b476470
Compare
| except: # anything, this it not crucial feature | ||
| return None | ||
|
|
||
| def was_warning_shown_recently(self) -> bool: |
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.
I'd separate this logic from _VersionCache - I imagine this class as a layer for reading/writing the cache file
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.
What would you suggest instead? I agree that the original intent of _VersionCache was probably to not hit pypi too often, but our requirements have changed. If we were very strict, the info when upgrade message was last displayed could go to another file, but that seems to be adding more complexity if you ask me. After all _last_time_shown is also related to the functionality of displaying version upgrade message, so I'd rather have that in a single file instead of worrying about management of another file.
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.
How about returning last_time_shown: Optional[int] and doing a comparison with a constant in get_new_version_msg?
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.
ok, that sounds like a good idea 👌
f36e526 to
058a39e
Compare
fd21b2e to
9aba625
Compare
…n in release notes
…ce version check tests
…ON_MESSAGE_INTERVAL
9aba625 to
9619c55
Compare
Pre-review checklist
Changes description
https://snowflakecomputing.atlassian.net/browse/SNOW-2131951