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

[sip-15] Displaying endpoints for all start/end time ranges #8817

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Dec 12, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR adds the SIP-15 endpoints to all time ranges which include a start and/or end as not just those containing a timestamp. This helps to provide more transparency. Note the conditional logic has mostly been reverted to what was changed here.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

AFTER

Screen Shot 2019-12-12 at 3 03 20 PM

Screen Shot 2019-12-12 at 3 03 31 PM

Screen Shot 2019-12-12 at 3 03 44 PM

Screen Shot 2019-12-12 at 3 11 36 PM

TEST PLAN

CI and manual testing.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

to: @etr2460 @graceguo-supercat @michellethomas

@codecov-io
Copy link

codecov-io commented Dec 12, 2019

Codecov Report

Merging #8817 into master will decrease coverage by 6.68%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8817      +/-   ##
==========================================
- Coverage   65.84%   59.16%   -6.69%     
==========================================
  Files         483      367     -116     
  Lines       24177    11679   -12498     
  Branches     2777     2862      +85     
==========================================
- Hits        15920     6910    -9010     
+ Misses       8079     4590    -3489     
- Partials      178      179       +1
Impacted Files Coverage Δ
.../explore/components/controls/DateFilterControl.jsx 38.78% <100%> (+0.46%) ⬆️
superset/assets/src/welcome/DashboardTable.jsx 75% <0%> (-12.5%) ⬇️
.../assets/src/SqlLab/components/AceEditorWrapper.jsx 55.69% <0%> (-4.08%) ⬇️
...src/dashboard/util/getFilterConfigsFromFormdata.js 88.46% <0%> (-2.45%) ⬇️
...uperset/assets/src/dashboard/components/Header.jsx 41.12% <0%> (-2.12%) ⬇️
...t/assets/src/dashboard/reducers/dashboardLayout.js 93% <0%> (-1.9%) ⬇️
...et/assets/src/dashboard/actions/dashboardLayout.js 95.09% <0%> (-1.88%) ⬇️
superset/assets/src/welcome/Welcome.jsx 78.57% <0%> (-1.43%) ⬇️
...ssets/src/dashboard/containers/DashboardHeader.jsx 100% <0%> (ø) ⬆️
...set/assets/src/dashboard/actions/dashboardState.js 40.96% <0%> (ø) ⬆️
... and 144 more

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 ed54f6e...af3fd53. Read the comment docs.

@mistercrunch mistercrunch added the sip Superset Improvement Proposal label Jan 7, 2020
@john-bodley
Copy link
Member Author

@john-bodley
Copy link
Member Author

ping @etr2460

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

accepting to unblock, but i think there's a cleaner way to do this

.map(
(v, idx) =>
(v.replace('T00:00:00', '') || (idx === 0 ? '-∞' : '∞')) +
(endpoints && value.includes(SEPARATOR)
Copy link
Member

Choose a reason for hiding this comment

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

instead of using value.includes here, i think it's cleaner to use the 3rd argument for the callback in map and check if the length of the array is > 1. see details here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map

john-bodley and others added 3 commits January 17, 2020 10:43
…rol.jsx

Co-Authored-By: Erik Ritter <erik.ritter@airbnb.com>
…rol.jsx

Co-Authored-By: Erik Ritter <erik.ritter@airbnb.com>
@etr2460 etr2460 merged commit 2fc5fd4 into apache:master Jan 17, 2020
@mistercrunch mistercrunch added this to APPROVED in SIPs Feb 24, 2020
@srinify srinify moved this from APPROVED to IMPLEMENTED / DONE in SIPs Feb 4, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels sip Superset Improvement Proposal size/S 🚢 0.36.0
Projects
Status: Implemented / Done
SIPs
IMPLEMENTED / DONE
Development

Successfully merging this pull request may close these issues.

None yet

4 participants