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

Fixed #29036 -- Change SelectDateWidget's empty value #9632

Merged
merged 1 commit into from Jan 31, 2018

Conversation

ziima
Copy link
Contributor

@ziima ziima commented Jan 30, 2018

@@ -1014,7 +1014,8 @@ def format_value(self, value):
year, month, day = d.year, d.month, d.day
match = self.date_re.match(value)
if match:
year, month, day = [int(val) for val in match.groups()]
# In case of pseudo-ISO dates, e.g. 2017-0-23, return empty string to match empty option value.
Copy link
Member

Choose a reason for hiding this comment

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

@ziima I know you didn't change this logic but, is this right?

It looks to me as if we're parsing value into a datetime and then parsing it again using date_re. (Or did I read that wrong?)

Shouldn't we be doing one or the other, and not both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was startled as well, the string is parsed twice. Since this wasn't part of the code which would be subject of the ticket, I left it unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

@ziima OK That's fine. Thanks.

I created a new ticket for the (query) logic error here. https://code.djangoproject.com/ticket/29089

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

I think this is fine. 👍

I had a slight query with regard to the addition of selected attribute for the empty case, e.g. in test_render_empty but I can't see a problem with it. (It's required for the tests to pass, and always behaves correctly.)

@timgraham timgraham force-pushed the 29036-empty-value-select-date-widget branch from a3e6c7d to f6c928b Compare January 30, 2018 23:48
…f the attribute is added by JavaScript.

Thanks Tim Graham for the initial patch.
@timgraham timgraham force-pushed the 29036-empty-value-select-date-widget branch from f6c928b to fbc3c29 Compare January 31, 2018 00:09
@timgraham timgraham merged commit fbc3c29 into django:master Jan 31, 2018
@ziima ziima deleted the 29036-empty-value-select-date-widget branch February 1, 2018 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants