Implemented number field formatter for durations #6499
Conversation
| ] | ||
| }]; | ||
|
|
||
| fixtures.forEach((fixture, i) => { |
There was a problem hiding this comment.
Is this pattern of dynamically generating test cases used for testing the other field formatters?
There was a problem hiding this comment.
I think other test files keep all assertions in a single test case
There was a problem hiding this comment.
One big test case isn't ideal either, but we should aim to be more explicit with our tests rather than generating them automatically. It's more boilerplate up front, but it pays off in test clarity and expressiveness down the line. As it stands now, I've read over this loop within a loop a few times, and I'm still not 100% confident I know what it's testing.
There was a problem hiding this comment.
I feel like this was a common enough pattern to avoid code repetition. I agree that the fixtures and inputs loop could be a bit clearer, though.
There was a problem hiding this comment.
Avoiding repetition is good, but clarity and expressiveness should be stronger concerns within tests. With some changes (namely not automating the test execution), you can still avoid duplicating the test case itself for common inputs/outputs and have expressive and concise tests.
One possible example of the same test cases: https://gist.github.com/epixa/aed0d611e41ac366073d
That all said, I'm pretty squarely in the don't-automate-test-creation camp, but there are others on the team that are more comfortable with it, so don't feel obligated to switch to the other extreme.
There was a problem hiding this comment.
To be fair, your code is much easier to read. I'll use a version of that.
There was a problem hiding this comment.
Rad. Be warned: my code wouldn't actually run or anything useful like that.
bevacqua
force-pushed
the
feature/duration-formatter
branch
from
19d38a9
to
c94261f
Compare
| import moment from 'moment'; | ||
| import IndexPatternsFieldFormatProvider from 'ui/index_patterns/_field_format/FieldFormat'; | ||
| import durationTemplate from 'ui/stringify/editors/duration.html'; | ||
| export default function DurationFormatProvider(Private) { |
There was a problem hiding this comment.
Can you add a space between the imports and export? It just makes it a little easier to grok at a glance.
There was a problem hiding this comment.
Same as above, virtually none of the files I saw do this. Thanks
|
Looking good. Go ahead and assign it back to me whenever you've addressed the feedback in either code or in comments. |
bevacqua
force-pushed
the
feature/duration-formatter
branch
from
c94261f
to
77f4dd2
Compare
|
|
||
| class Duration extends FieldFormat { | ||
| constructor(params) { | ||
| super(params); |
There was a problem hiding this comment.
There's no need to specify a constructor if it's only calling the parent constructor.
|
Added a few things, but this looks almost ready. |
bevacqua
force-pushed
the
feature/duration-formatter
branch
from
77f4dd2
to
19bf209
Compare
|
That should do it. |
|
Assuming tests go green, LGTM. At this point I'd assign it back to you and you'd grab a second person to review it. We can sort it out on irc though. |
|
LGTM |
Implemented number field formatter for durations
|
🎆 |
This PR fixes #5130.
Numeric fields in elasticsearch can represent a duration. This PR adds a flexible
Durationformatter that allows the user to decide how to display durations.seconds)humanize)2)