Handle "ambiguous durations" #5785

betodealmeida · 2018-08-31T06:04:52Z

The play slider currently doesn't handle "ambiguous durations" like P1M (1 month) or P1Y (1 year), since these can't be converted into seconds by the module parse-iso-duration. I created a wrapper class that correctly handles these cases, incrementing the month/year number instead of adding a fixed number of seconds.

kristw · 2018-08-31T14:41:32Z

superset/assets/src/modules/time.js

@@ -1,38 +1,155 @@
 import parseIsoDuration from 'parse-iso-duration';


+export class IsoDuration {


Perhaps create a new file called IsoDuration.

kristw · 2018-08-31T14:48:45Z

superset/assets/src/modules/time.js

+    // special cases not handled by parse-iso-duration
+    this.pattern = /P(\d+)(M|Y)/;
+
+    this.addTo = this.addTo.bind(this);


no need to bind

kristw · 2018-08-31T14:49:00Z

superset/assets/src/modules/time.js

+    this.isoDuration = isoDuration;
+
+    // special cases not handled by parse-iso-duration
+    this.pattern = /P(\d+)(M|Y)/;


If this is a constant, declare outside class

kristw · 2018-08-31T14:57:24Z

superset/assets/src/visualizations/deckgl/AnimatableDeckGLContainer.jsx

@@ -8,7 +8,7 @@ const propTypes = {
  getLayers: PropTypes.func.isRequired,
  start: PropTypes.number.isRequired,
  end: PropTypes.number.isRequired,
-  step: PropTypes.number.isRequired,
+  step: PropTypes.object,


step: PropTypes.instanceOf(IsoDuration)

kristw · 2018-08-31T15:02:42Z

superset/assets/src/modules/time.js

+    let foundDifference = false;
+    const args = [];
+    const lowest = [1, 1, 1, 0, 0, 0, 0];
+    for (let i = 0; i < explodedTimestamp.length; i++) {


Avoid naming the variable args as it is usually used for the entire function arguments.

Can also simplify the code

const firstDiffIndex = explodedTimestamp .map((part, i) => (explodedLowerBound[i] !== part)) .indexOf(true); const dateParts = firstDiffIndex > -1 ? explodedTimestamp.map((part, i) => i >= firstDiffIndex ? lowest[i] : part) : explodedTimestamp; return Date.UTC(...dateParts)

kristw · 2018-08-31T15:03:56Z

superset/assets/src/modules/time.js

+    const explodedLowerBound = this.explode(lowerBound);
+    let foundDifference = false;
+    const args = [];
+    const lowest = [1, 1, 1, 0, 0, 0, 0];


If this is a constant, please declare outside the class. Perhaps choose a bit more descriptive name or add comment what lowest means?

codecov-io · 2018-09-01T01:49:33Z

Codecov Report

Merging #5785 into master will increase coverage by 0.03%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #5785      +/-   ##
==========================================
+ Coverage   63.78%   63.82%   +0.03%     
==========================================
  Files         364      364              
  Lines       23111    23138      +27     
  Branches     2587     2589       +2     
==========================================
+ Hits        14741    14767      +26     
- Misses       8355     8356       +1     
  Partials       15       15

Impacted Files	Coverage Δ
...ts/src/visualizations/deckgl/layers/screengrid.jsx	`0% <0%> (ø)`	⬆️
...isualizations/deckgl/AnimatableDeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
...sualizations/deckgl/CategoricalDeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/modules/time.js	`93.75% <93.02%> (+3.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a4b1b7...9e30fc7. Read the comment docs.

mistercrunch · 2018-09-10T17:28:14Z

LGTM

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 4a9274c40c17e2a12d97cf28074a1e876dee732f) (cherry picked from commit c57f772)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 4a9274c40c17e2a12d97cf28074a1e876dee732f)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 4a9274c40c17e2a12d97cf28074a1e876dee732f) (cherry picked from commit c57f772)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 4a9274c40c17e2a12d97cf28074a1e876dee732f)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 4a9274c40c17e2a12d97cf28074a1e876dee732f) (cherry picked from commit c57f772)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 25ac26d)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props (cherry picked from commit f740974) (cherry picked from commit 25ac26d)

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props

betodealmeida added 5 commits August 30, 2018 22:53

WIP

f5374db

Update interface

536c47b

Fix truncate

4416f75

Improve unit tests

591d68e

Improve code

cfbfb13

kristw reviewed Aug 31, 2018

View reviewed changes

betodealmeida added 2 commits August 31, 2018 18:30

Use moment.js to parse ISO durations

f15ffb3

Fix typo and React props

991ce94

betodealmeida added 2 commits August 31, 2018 21:15

Fix conflict

9e30fc7

Rebase and fix conflict

df264e6

mistercrunch merged commit f740974 into apache:master Sep 10, 2018

mistercrunch deleted the DPTOOLS-1014 branch September 10, 2018 17:28

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Handle "ambiguous durations" (apache#5785)

895e060

* WIP * Update interface * Fix truncate * Improve unit tests * Improve code * Use moment.js to parse ISO durations * Fix typo and React props

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle "ambiguous durations" #5785

Handle "ambiguous durations" #5785

betodealmeida commented Aug 31, 2018

kristw Aug 31, 2018

kristw Aug 31, 2018

kristw Aug 31, 2018

kristw Aug 31, 2018

kristw Aug 31, 2018 •

edited

Loading

kristw Aug 31, 2018

codecov-io commented Sep 1, 2018 •

edited

Loading

mistercrunch commented Sep 10, 2018

		@@ -1,38 +1,155 @@
		import parseIsoDuration from 'parse-iso-duration';


		export class IsoDuration {

Handle "ambiguous durations" #5785

Handle "ambiguous durations" #5785

Conversation

betodealmeida commented Aug 31, 2018

kristw Aug 31, 2018

Choose a reason for hiding this comment

kristw Aug 31, 2018

Choose a reason for hiding this comment

kristw Aug 31, 2018

Choose a reason for hiding this comment

kristw Aug 31, 2018

Choose a reason for hiding this comment

kristw Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

kristw Aug 31, 2018

Choose a reason for hiding this comment

codecov-io commented Sep 1, 2018 • edited Loading

Codecov Report

mistercrunch commented Sep 10, 2018

kristw Aug 31, 2018 •

edited

Loading

codecov-io commented Sep 1, 2018 •

edited

Loading