Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DurationField output format #8532
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
DurationField output format #8532
Changes from all commits
5723ba1
c3efa55
4377b08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 find the use of the term "format string" very confusing here. The term means something else in Python, referring to string with placeholder variables that get filled in. That sounds like one could set
format="%h:%m:%s"
to get the duration in hours, minutes, seconds, but that is not the case. Please find another word, like "format family", "format variety", ... or something.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.
Good point, how about
format_name
?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.
sounds good!
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 used the "format" name just to keep uniformity with date/time fields, because to me it looked more elegant.
Since there is nothing like strftime/strptime for
timedelta
this may also just be a flag if it is clear/readable enough.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.
What output style does this produce with the current JSONRenderer?
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.
Using the current
JSONRenderer
, which usesJSONEncoder
it will output the string value of "total seconds" (doc).django-rest-framework/rest_framework/utils/encoders.py
Lines 39 to 40 in 129890a
Using the same example value will be:
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 turns out that this value isn't used anywhere; as long as
format
is set to anything else butISO_8601
, we end up with'standard'
behavior.That's probably not intended. Can this be correct? Let's also add a test case for the behavior with an unknown
format
value.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.
That value was placed just to be a placeholder for the default format. It may just be set to
None
insettings.py
. I added the "standard" after this conversation just to improve readability. The format-as-ISO may also be a flag, however that pattern may be harder to read and would add a non-uniformity with other time-related fields.It is true that django itself does not provide much flexibility with duration representation, which also comes from stdlib
timedelta
which has no implementation with to-str or from-str like date/time does.Which way do you prefer for completing this?
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.
We should disallow specifying this as positional argument:
DurationField('iso-8601')
(we don't want that).All other options come from the
**kwargs
...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.
Would be nice to add test coverage for parsing duration in ISO format, as I would expect it to work both ways
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.
Currently the
parse_duration
method of django, which is used insideDurationField
, already parses ISO-8601 format and the format specific to postgres by its own.This could be added to the
TestDurationField
testcase.Where do you prefer these tests to be added?
PS:
From this point of view the current error message
"Use one of these formats instead: [DD] [HH:[MM:]]ss[.uuuuuu]"
is not (completely) right, however changing it would be a breaking change.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'm not sure where that an extension of this error message should be considered a breaking change; the wording "one of" already hints that the application should not rely the one specific format being given in the message.