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

Differentiate between datetimeformat and dateformat #869

Merged
merged 6 commits into from Sep 22, 2018

Conversation

Projects
None yet
5 participants
@knagra
Contributor

knagra commented Jul 5, 2018

Marshmallow currently uses dateformat in the DateTime field and does not support a custom format for Date. This PR changes DateTime.dateformat to DateTime.DateTimeFormat and adds Date.dateformat.

This is my first PR for this repo. Please let me know if I'm breaking any protocols or if other changes are necessary for this to be merged.

@@ -1016,32 +1027,86 @@ def _deserialize(self, value, attr, data):
class Date(Field):
"""ISO8601-formatted date string.
:param strict: If True, only accept Date objets. If False, will accept any

This comment has been minimized.

@sloria

sloria Jul 10, 2018

Member

Drive-by: There is some discussion about removing strict flags on fields here: #755 (comment) , so I have reservations about adding it to Date.

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

Thanks @knagra for the PR. I think you're onto something here. I don't have time to review this deeply right now, but that's not to say it won't get reviewed/merged. I'm currently focused on moving marshmallow 3 forward, which means reviewing backwards-incompatible API changes.

If anyone else would like to review this, all feedback is welcome.

@knagra

This comment has been minimized.

Contributor

knagra commented Jul 10, 2018

I can remove the strict flag. Also, this is a backwards-incompatible change. Currently, DateTimeField uses dateformat in meta. Under these changes, DateTimeField will use datetimeformat in meta and DateField will use dateformat. Thanks for the quick initial look, @sloria.

@sloria

This comment has been minimized.

Member

sloria commented Jul 11, 2018

Yeah, go ahead and remove the strict option for now. That can be discussed separate from this PR. I'm good with the overall approach here. Since this is a breaking change, I'd like at least one more review of this. cc @deckar01 @lafrech

@sloria sloria added this to the 3.0 milestone Jul 11, 2018

self.fail('format', input=value)
return value
def _serialize(self, value, attr, obj):

This comment has been minimized.

@deckar01

deckar01 Jul 11, 2018

Member

A lot of this new code seems to be duplicated from DateTime. Can we use inheritance to avoid duplicating this code?

This comment has been minimized.

@knagra

knagra Jul 12, 2018

Contributor

The only major impediment is the distinction between dateformat and datetimeformat in Meta, but I think I can write a function to change that between the two classes.

How are you envisioning the class inheritance graph for this? class Date(DateTime) or class Date(FormattedField) and class DateTime(FormattedField) or something similar?

This comment has been minimized.

@deckar01

deckar01 Jul 12, 2018

Member

I don't think python has a defined hierarchy between the two modules. I think it would be valid to treat Date as a specialized form of DateTime. class DateTime(Field) and class Date(DateTime) would probably generate the smallest diff also.

I can write a function to change [the meta format] between the two classes

👍 Good catch.

def __init__(self, strict=False, format=None, **kwargs):
super(Date, self).__init__(**kwargs)
self.strict = strict
self.dateformat = format

This comment has been minimized.

@deckar01

deckar01 Jul 11, 2018

Member

It might be better to avoid field name specific prefixes to make these properties nicer to override with inheritance. They are already scoped under a class, so there is no benefit to further namespacing them.

  • self.dateformat -> self.format
  • DATEFORMAT_SERIALIZATION_FUNCS -> SERIALIZATION_FUNCS
  • DATEFORMAT_DESERIALIZATION_FUNCS -> DESERIALIZATION_FUNCS

This comment has been minimized.

@knagra

knagra Jul 12, 2018

Contributor

That makes sense to me. I'll make the changes.

@@ -309,6 +309,11 @@ def from_iso_date(datestring, use_dateutil=True):
else:
return datetime.datetime.strptime(datestring[:10], '%Y-%m-%d').date()
def to_iso_date(date):
return date.strftime('%Y-%m-%d')

This comment has been minimized.

@fuhrysteve

fuhrysteve Jul 12, 2018

This functionality already exists in the stdlib as datetime.date.isoformat - so we should be able to use that instead of adding another utils function.

In [1]: import datetime

In [2]: datetime.date.today().isoformat()
Out[2]: '2018-07-12'

In [3]: datetime.date.isoformat(datetime.date.today())
Out[3]: '2018-07-12'

This comment has been minimized.

@knagra

knagra Jul 12, 2018

Contributor

I'm fine with that. I'll make the change.

@lafrech

This comment has been minimized.

Member

lafrech commented Sep 2, 2018

@knagra thanks for this work, already. Are you still working on this?

@knagra

This comment has been minimized.

Contributor

knagra commented Sep 2, 2018

Yeah, I'm about to work on this today and tomorrow. Just been busy with work.

@lafrech

This comment has been minimized.

Member

lafrech commented Sep 2, 2018

No pb, no pressure. Thanks.

@knagra knagra force-pushed the knagra:knagra/date-field-fix branch 3 times, most recently from 647beb1 to e865cdf Sep 2, 2018

@knagra knagra force-pushed the knagra:knagra/date-field-fix branch from e865cdf to 8efb79a Sep 3, 2018

@knagra

This comment has been minimized.

Contributor

knagra commented Sep 3, 2018

@deckar01 @fuhrysteve I've incorporated your suggested changes. Implementing Date(DateTime) inheritance required adding a couple of extra class variables to those fields to maintain cleanliness of error messages and to allow schemas to have both dateformat and datetimeformat in the Meta class.

I converted the to_iso_format function to use datetime.date.isoformat as suggested but decided to leave to function to absorb the extra self.localtime keyword argument passed to it by DateTime.

Let me know if you have other suggestions. I should be able to address them more quickly now.

def _serialize(self, value, attr, obj):
self.format = self.format or self.DEFAULT_FORMAT

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

This will conflict with #934.

Please don't mutate self.format and use a local format variable.

This comment has been minimized.

@knagra

knagra Sep 5, 2018

Contributor

Okay. It seems that change was merged just a few days ago. I'll go ahead and change self.format to just format in this case. Do you want me to change the name of the self.format variable back to self.dateformat as well? I changed it since dateformat in the DateTime field doesn't make sense.

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

It seems that change was merged just a few days ago. I'll go ahead and change self.format to just format in this case.

Yes, exactly. I though I'd mention that to help you with the rebase.

Do you want me to change the name of the self.format variable back to self.dateformat as well?

No, not at all. format is better now.

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

Sorry, I just realized this PR is rebased already. Then please change self.format to format as in #934.

assert serialized['created'] == user.created.strftime(fmt)
assert serialized['updated'] == user.updated.strftime('%m-%d')
def test_dateformat_option(user):
fmt = '%Y-%m'

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

Very minor: it is strange to define this format here and no the other one.

This comment has been minimized.

@knagra

knagra Sep 5, 2018

Contributor

It was the pattern saw I above at line 1471. I'll go ahead name them in both tests.

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

Oh, right, of course. Thanks. No pb otherwise.

except TypeError:
raise self.fail('invalid')
parsed = utils.from_datestring(value)
return dt.datetime(

This comment has been minimized.

@lafrech

lafrech Sep 5, 2018

Member

I don't understand this change. I don't think this would return a dt.date when called from Date.

Isn't this code unreachable anyway ? (#758)

(I don't mean to throw #758 in your way, this is another story. If the code is indeed unreachable, which I think it is, don't worry too much about it. I just don't understand the change.)

This comment has been minimized.

@knagra

knagra Sep 11, 2018

Contributor

The object creation is an oversight. I'll fix that. I think #758 is beyond the scope of this PR. I can look at it after this change, though, if you need someone to work on it.

@knagra

This comment has been minimized.

Contributor

knagra commented Sep 11, 2018

@lafrech Thanks for the review. I'll implement changes tonight and have it ready for another take.

@knagra

This comment has been minimized.

Contributor

knagra commented Sep 12, 2018

@lafrech I implemented your suggestions. Yes, the code block you mentioned is inaccessible. That's why we weren't getting any errors from the tests. I don't think there's a clear quick fix for #758. There are some behavior questions that need to be answered.

@lafrech lafrech force-pushed the knagra:knagra/date-field-fix branch from 9d9c9b5 to dcf7967 Sep 12, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Sep 12, 2018

Thanks @knagra for your work on this.

I think this is ready to merge.

@lafrech

This comment has been minimized.

Member

lafrech commented Sep 12, 2018

@knagra, if you have time, would you mind adding a paragraph to docs/upgrading.rst?

@lafrech lafrech added the datetime label Sep 22, 2018

@lafrech lafrech merged commit cd0cfc7 into marshmallow-code:dev Sep 22, 2018

1 check passed

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

This comment has been minimized.

Member

lafrech commented Sep 22, 2018

I merged and updated the docs.

Thanks again @knagra. Please double-check the doc update if you get the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment