-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix inactive DatePicker if the date value is 10 years earlier #3241
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
Conversation
…nput to avoid inactive widget when initial date out of range [-10 year, +10 year] from today
… 29) problem Add unit tests
| [ | ||
| (date(1961, 4, 12), "1951/04/12", "1971/04/12"), | ||
| (date(2020, 2, 29), "2010/02/28", "2030/02/28"), | ||
| # TODO: Find a way to mock date.today() |
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 haven't been able to find a way to mock the date.today() function
A possible solution is to use the freezgun library.
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.
It looks like there's an example in the unittest.mock documentation that provides a way to mock date.today: https://docs.python.org/3/library/unittest.mock-examples.html#partial-mocking, which will require a separate test be written since I doubt it'll be possible to combine this approach with using parametrize()
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.
@vdonato I tried this option and several options with a modified patch path
Unfortunately, everything falls with errors
For example this:
def test_min_max_values_missing_value(self):
with patch("streamlit.delta_generator.DeltaGenerator.date_input.date") as mock_date:
mock_date.today.return_value = date(1999, 10, 8)
mock_date.side_effect = lambda *args, **kw: date(*args, **kw)
st.date_input("the label", [])
c = self.get_delta_from_queue().new_element.date_input
self.assertEqual(c.min, "1989/10/08")
self.assertEqual(c.max, "2009/10/08")
fails with error
AttributeError: <function TimeWidgetsMixin.date_input at 0x1526e48b0> does not have the attribute 'date'
I think we will find a way to mock dates in tests. Or we will start using a library for this.
For now, I will leave a TODO comment, to come back to this later.
| min_value : datetime.date or datetime.datetime | ||
| The minimum selectable date. Defaults to today-10y. | ||
| The minimum selectable date. Defaults to value - 10 years or left | ||
| boundary of interval - 10 years, if value is а range. |
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.
Please pay attention to the docstring.
Do we need to update the documentation somewhere else?
Does this docstring line explain the behavior well enough, or is it better to rephrase?
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.
The documentation should be updated automatically (as long as this docstring is already in the existing docs, which it is) since docs in our API reference are rendered from the docstrings themselves.
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.
As for the documentation wording, I think this is fine, but it may be slightly clearer to phrase it as something like
"The minimum selectable date. If value is a date, defaults to value - 10 years. If value is the interval [start, end], defaults to start - 10 years."
| [ | ||
| (date(1961, 4, 12), "1951/04/12", "1971/04/12"), | ||
| (date(2020, 2, 29), "2010/02/28", "2030/02/28"), | ||
| # TODO: Find a way to mock date.today() |
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.
It looks like there's an example in the unittest.mock documentation that provides a way to mock date.today: https://docs.python.org/3/library/unittest.mock-examples.html#partial-mocking, which will require a separate test be written since I doubt it'll be possible to combine this approach with using parametrize()
| min_value : datetime.date or datetime.datetime | ||
| The minimum selectable date. Defaults to today-10y. | ||
| The minimum selectable date. Defaults to value - 10 years or left | ||
| boundary of interval - 10 years, if value is а range. |
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.
The documentation should be updated automatically (as long as this docstring is already in the existing docs, which it is) since docs in our API reference are rendered from the docstrings themselves.
| min_value : datetime.date or datetime.datetime | ||
| The minimum selectable date. Defaults to today-10y. | ||
| The minimum selectable date. Defaults to value - 10 years or left | ||
| boundary of interval - 10 years, if value is а range. |
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.
As for the documentation wording, I think this is fine, but it may be slightly clearer to phrase it as something like
"The minimum selectable date. If value is a date, defaults to value - 10 years. If value is the interval [start, end], defaults to start - 10 years."
* develop: Fix inactive DatePicker if the date value is 10 years earlier (streamlit#3241) Upgrade trim to 0.0.3 or later (streamlit#3250) Manually name all snapshots and generate missing ones (streamlit#3205)
* develop: (75 commits) Remove tag from PR template (#3284) Add test req version bound for TF to fix tests (#3266) pin click to < 8.0 (#3256) Up version to 0.82.0 Update change log Add support for toml syntax highlighting in code blocks (#3140) Don't allow config.get_option to be called on file import (#3235) pin click to < 8.0 (#3256) Random cleanup: docstrings + type-safety (#3252) "setWidgetValue" -> "commitWidgetValue" (#3253) Fix inactive DatePicker if the date value is 10 years earlier (#3241) Upgrade trim to 0.0.3 or later (#3250) Manually name all snapshots and generate missing ones (#3205) Fix Video Recorded modal dialog width (#3226) Bump handlebars from 4.7.6 to 4.7.7 in /frontend (#3238) Bump hosted-git-info from 2.8.8 to 2.8.9 in /frontend (#3234) Bump lodash from 4.17.20 to 4.17.21 in /frontend (#3233) Bump ua-parser-js from 0.7.23 to 0.7.28 in /frontend (#3231) Update FAQs with supported browsers (#3225) Update copyright date (#3228) ...
Issue: #2731
Now we compute min_value and max_value for the DatePicker widget dynamically based on provided value or values range.
relativedeltaused instead of increasing year by 10, to avoid crash when current date is Feb 29 of leap year