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

Add DurationField #2989

Merged
merged 1 commit into from Jun 1, 2015
Merged

Add DurationField #2989

merged 1 commit into from Jun 1, 2015

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Jun 1, 2015

Field that map to django.db.models.fields.DurationField for Django>=1.8 otherwise
with django.db.models.fields.BigIntegerField

@xordoquy xordoquy added this to the 3.1.3 Release milestone Jun 1, 2015
@tomchristie
Copy link
Member

tomchristie commented Jun 1, 2015

Refs #2481.


A Duration representation.
Corresponds to `django.db.models.fields.Duration` for Django>=1.8
otherwise to `django.db.models.fields.BigIntegerField`.
Copy link
Member

@tomchristie tomchristie Jun 1, 2015

Choose a reason for hiding this comment

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

The documentation for this needs to go into what the input/output representation is.
Also what internal value is used (a timedelta, right?)

Copy link
Contributor Author

@ticosax ticosax Jun 1, 2015

Choose a reason for hiding this comment

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

Yes it is a timedelta.
I will update the documentation

Copy link
Contributor Author

@ticosax ticosax Jun 1, 2015

Choose a reason for hiding this comment

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

@tomchristie I tried to find a better place for the documentation but I couldn't find it.
Please, Can you explain in details what you expect ?

Copy link
Member

@tomchristie tomchristie Jun 1, 2015

Choose a reason for hiding this comment

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

I would simply mention that the validated_data for these fields will contain a timedelta, and that the field is only available with Django versions >= 1.8

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

I realized something is missing for Backward compatibility. I need to transform the timedelta instance into microseconds in DurationField(BingIntegerField) .
please hold the merge

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

I pushed a new version converting timedeltainto microseconds for rest_framework.compat.DurationField.

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

@tomchristie - given the backported wall of code -for DurationField- wouldn't it be simpler if we don't provide the DurationField if Django < 1.8 ?
I'm thinking about how would we reflect the Django's core changes to this code ?

@tomchristie
Copy link
Member

tomchristie commented Jun 1, 2015

wouldn't it be simpler if we don't provide the DurationField if Django < 1.8

Sounds reasonable.

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

ok, I'll remove backport of DurationField

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

@ticosax thanks a lot and sorry for the extra work.

A Duration representation.
Corresponds to `django.db.models.fields.Duration` for Django>=1.8
otherwise to `django.db.models.fields.BigIntegerField`.
The representation is a string following this format `'[DD] [HH:[MM:]]ss[.uuuuuu]'`
Copy link
Member

@tomchristie tomchristie Jun 1, 2015

Choose a reason for hiding this comment

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

Missing a full stop at the end of this sentence. (Just after the final backtick)

@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

I removed the backport of DurationField.
It will now raise NotImplementedError if someone tries to use DurationField with django < 1.8

@tomchristie
Copy link
Member

tomchristie commented Jun 1, 2015

Looks really nice now!

@tomchristie
Copy link
Member

tomchristie commented Jun 1, 2015

@xordoquy I'm happy with this whenever you are.

@xordoquy
Copy link
Collaborator

xordoquy commented Jun 1, 2015

Brillant. Thanks a lot @ticosax !!

xordoquy added a commit that referenced this pull request Jun 1, 2015
@xordoquy xordoquy merged commit 14055dd into encode:master Jun 1, 2015
1 check passed
@ticosax
Copy link
Contributor Author

ticosax commented Jun 1, 2015

That was fast ! thank you also.
hoping for the new release now

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

Successfully merging this pull request may close these issues.

None yet

3 participants