Global datetime formatter#143
Global datetime formatter#143mcclayton merged 10 commits intoprocore-oss:masterfrom ritikesh:datetime_formatter
Conversation
mcclayton
left a comment
mcclayton
left a comment
There was a problem hiding this comment.
Thanks for making these changes! I’ll make the CHANGELOG updates at the time of the next release. Just want @philipqnguyen to get a chance to look at this and I’ll merge if these changes look good to him too 👍
philipqnguyen
left a comment
philipqnguyen
left a comment
There was a problem hiding this comment.
Thank you!!! We really needed this. I have some comments that I would like to be addressed or at least discussed about.
| end | ||
| end | ||
| it('raises a BlueprinterError') { expect{subject}.to raise_error(Blueprinter::BlueprinterError) } | ||
| it('does not apply the date format') { should eq(result) } |
There was a problem hiding this comment.
This is technically a breaking change. Previously we promised to raise an error if they passed an object with a datetime_format option that did not respond to #strftime, but now we are returning the original value.
| if value.respond_to?(:strftime) | ||
| value = format_datetime(value, options) | ||
| end | ||
| value |
There was a problem hiding this comment.
I think we need to alter the logic here a bit in order to prevent a breaking change.
- If
options[:datetime_format]is explicitly passed and!value.respond_to?(:strftime), we should raise the originalBlueprinterError. - Otherwise, follow through with the rest of this logic.
| extraction = extractor(object, options).extract(field_name, object, local_options, options) | ||
| value = options.key?(:datetime_format) ? format_datetime(extraction, options[:datetime_format]) : extraction | ||
|
|
||
| value = @datetime_formatter.extract(extraction, options) |
There was a problem hiding this comment.
In order to follow the current naming paradigm, how do you feel if we renamed DateTimeFormatter#extract to DateTimeFormatter#format? Since #extract makes more sense with the Extractor classes, but not so for this Formatter class.
philipqnguyen
left a comment
philipqnguyen
left a comment
There was a problem hiding this comment.
Thank you for making the changes I requested @ritikesh . Looks good to me.
3 participants
No description provided.