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

chore: make config ENABLE_REACT_CRUD_VIEWS = True by default #11259

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

mistercrunch
Copy link
Member

React CRUD views have been maturing for a while and are ready for prime time.

Screen Shot 2020-10-13 at 8 01 41 PM

React CRUD views have been maturing for a while and are ready for prime
time!
@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #11259 into master will increase coverage by 4.87%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11259      +/-   ##
==========================================
+ Coverage   60.70%   65.57%   +4.87%     
==========================================
  Files         393      831     +438     
  Lines       24820    39511   +14691     
  Branches        0     3598    +3598     
==========================================
+ Hits        15066    25908   +10842     
- Misses       9754    13494    +3740     
- Partials        0      109     +109     
Flag Coverage Δ
#cypress 55.97% <ø> (?)
#javascript 62.67% <ø> (?)
#python 60.74% <50.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/config.py 90.11% <50.00%> (ø)
superset/db_engine_specs/presto.py 81.42% <0.00%> (-0.65%) ⬇️
...uperset-frontend/src/components/PopoverSection.jsx 100.00% <0.00%> (ø)
...frontend/src/dashboard/util/newEntitiesFromDrop.js 100.00% <0.00%> (ø)
...-frontend/src/common/components/common.stories.tsx 0.00% <0.00%> (ø)
...ntend/src/explore/components/ExploreChartPanel.jsx 75.00% <0.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 100.00% <0.00%> (ø)
...nd/src/dashboard/util/findTabIndexByComponentId.js 92.85% <0.00%> (ø)
superset-frontend/src/reduxUtils.ts 79.74% <0.00%> (ø)
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx 72.72% <0.00%> (ø)
... and 433 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 2c649ac...78581fa. Read the comment docs.

@etr2460
Copy link
Member

etr2460 commented Oct 14, 2020

@mistercrunch mind holding off on this for a day or two for us to verify in our environment that everything works as expected? We might mind some further bugs worth addressing before making it the community wide default (although I know we can always simply disable it too)

@mistercrunch
Copy link
Member Author

Sounds good @etr2460 , this PR will be right here ready to merge :)

@bkyryliuk
Copy link
Member

excited about this change ! we have been using react views for quite some time at dropbox

@etr2460
Copy link
Member

etr2460 commented Oct 14, 2020

Thanks @mistercrunch! I added a few bugs I found through a precursory QA pass, but otherwise it looks much more feature complete than a few months ago. You can decide which of these (if any) are blocking, then feel free to merge!

#11266 #11265 #11264 #11263

@mistercrunch
Copy link
Member Author

@nytai ^^^

@mistercrunch
Copy link
Member Author

It appears only these two are still open:

Overall the new experience is much better and been used in production at scale in many environments. It's also mentioned in UPDATING.md as a reminder. Merign!

@mistercrunch mistercrunch merged commit a1f8429 into apache:master Oct 19, 2020
@mistercrunch mistercrunch deleted the ENABLE_REACT_CRUD_VIEWS branch October 19, 2020 04:37
@kgabryje
Copy link
Member

@mistercrunch Maybe we should modify tests/superset_test_config.py to have ENABLE_REACT_CRUD_VIEWS true by default as well?

@kkucharc
Copy link
Contributor

kkucharc commented Oct 19, 2020

Hi @mistercrunch I think this change caused problems in pre-commit mypy:

superset/config.py:959: error: Incompatible import of "CELERY_CONFIG" (imported name has type "Type[superset_config.CeleryConfig]", local name has type "Type[superset.config.CeleryConfig]")

superset/config.py:959: error: Incompatible import of "CeleryConfig" (imported name has type "Type[superset_config.CeleryConfig]", local name has type "Type[superset.config.CeleryConfig]")

Problem appears only when pre-commit run --all-files is run.

@mistercrunch
Copy link
Member Author

revert? unclear why mypy was triggering/flaking for me, it seems like depending on environment mypy doesn't agree with itself...

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
…11259)

React CRUD views have been maturing for a while and are ready for prime
time!
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 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 size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants