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 orm_item_locator to use dateutil #1298

Merged
merged 19 commits into from Jun 17, 2019

Conversation

@yeojin-dev
Copy link
Contributor

commented Feb 4, 2019

Issue #1230
I think the issue is caused by tzinfo=<UTC>. Because <UTC> isn't UTC, tzinfo instance.
In case of ForeignKey and M2MField, it's ok to make datetime as string.

@coveralls

This comment has been minimized.

Copy link

commented Feb 4, 2019

Coverage Status

Coverage increased (+0.3%) to 71.353% when pulling 73f1126 on yeojin-dev:patch-dumpscript into 9bce928 on django-extensions:master.

@trbs

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Why not implement it as: "dateutil.parser.parse(\"%s\")" % value.isoformat() such is done in get_attribute_value(...) ?

@yeojin-dev yeojin-dev changed the title Fix orm_item_locator for not making datetime instance Fix orm_item_locator to use dateutil Feb 15, 2019

@yeojin-dev

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Sorry. I misunderstood. Could you see my new commit? @trbs

@trbs

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Thanks @yeojin-dev !

One last question; could you amend your last patch to use:

    clean_dict[key] = "dateutil.parser.parse(\"%s\")" % v.isoformat()

As I think this would fail for timezone aware datetimes. Those would look like datetime.datetime(2003, 10, 11, 17, 13, 46, tzinfo=tzoffset(None, 7200)) and the tzinfo items like tzoffset, tzlocal, tzutc, etc are not imported.

@yeojin-dev

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2019

Thank you for reviewing! I've fixed it. @trbs

@trbs

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Thanks @yeojin-dev for all your work on this PR.

I hate to say it but i would prefer to merge the other solution from my comment. As this is the same as we use elsewhere in the command and does not change the objects timezone.

@MRigal
Copy link

left a comment

@trbs I think your concerns are not justified

As I think this would fail for timezone aware datetimes. Those would look like datetime.datetime(2003, 10, 11, 17, 13, 46, tzinfo=tzoffset(None, 7200)) and the tzinfo items like tzoffset, tzlocal, tzutc, etc are not imported.

Please see https://docs.python.org/3/library/datetime.html#datetime.datetime.isoformat

Besides that, as commented, I would rather use make_aware

django_extensions/management/commands/dumpscript.py Outdated Show resolved Hide resolved

yeojin-dev added some commits May 19, 2019

@trbs

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Thanks @yeojin-dev could you elaborate a little bit on why the datetimes_to_replace list is needed ? (from first glance it's not really obvious)

@yeojin-dev

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Codes may behave incorrectly when there are multiple datetime fields. It is necessary for a datetime to be changed exactly one by one into a dateutil.

tests/testapp/settings.py Outdated Show resolved Hide resolved
@trbs

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Codes may behave incorrectly when there are multiple datetime fields. It is necessary for a datetime to be changed exactly one by one into a dateutil.

I am sorry, I still do not really understand why this is needed. Also using number + space + isoformat placeholder and then doing a big search + replace over the output does not feel right..

I would like to understand why this is the case and if we cannot solve it in a more elegant way.

@yeojin-dev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

Sorry for the late reply. I fixed it. Would you please review my code? @trbs

@trbs

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@yeojin-dev thanks for all your hard work on this !

PR looks good to me :-)

@trbs trbs merged commit e6df117 into django-extensions:master Jun 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yeojin-dev yeojin-dev deleted the yeojin-dev:patch-dumpscript branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.