You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/01/26 00:50:59 UTC
[GitHub] [incubator-superset] nytai opened a new pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
nytai opened a new pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025
…r CRUD views
### CATEGORY
Choose one
- [x] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
As the recent work to replace the FAB CRUD views does not support all the features provided by FAB (mainly, the various filters), a config option is necessary. This config allows for the server rendered CRUD view replacement work to proceed in master without a user facing loss of functionality. Users can also optionally enable the new react views.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
- toggling ENABLE_REACT_CRUD_VIEWS on renders the new react list view.
- toggling ENABLE_REACT_CRUD_VIEWS off renders the old FAB CRUD list view.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
@etr2460 @dpgaspar @mistercrunch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371413976
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
was just following the first pattern I saw, makes no difference to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda merged pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
craig-rueda merged pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371413976
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
was just following the first pattern I saw, makes no difference to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371335155
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
is there a reason why we don't reference the app from inside the function instead of pulling the config out at global scope? I know i've seen this pattern a lot, but we've run into issues with tests because of it in the past. do you think it might be better to move it to the function? cc @john-bodley for his thoughts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371363184
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
I think this is used for connivence though in general I’m not certain the abstraction is worth it. I would prefer we use `app.config[...]` on line #62.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371363184
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
I think this is used for connivence though in general I’m not certain the abstraction is worth it. I would prefer we use `app.config[...]` on line #62. This is more of a style nit and thus is non-blocking.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371363184
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
I think this is used for connivence though in general I’m not certain the abstraction/indirection is worth it. I would prefer we use `app.config[...]` on line #62 as it’s clearer what the `config` variable represents. This is more of a style nit and thus is non-blocking.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#issuecomment-578458338
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/68e85ab1b6ba788504f8ced93c22077b3670d25a?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11686 11686
Branches 2866 2866
=======================================
Hits 6913 6913
Misses 4594 4594
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=footer). Last update [68e85ab...b3d8c4f](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#discussion_r371363184
##########
File path: superset/views/dashboard/views.py
##########
@@ -40,6 +40,8 @@
from ..utils import bootstrap_user_data
from .mixin import DashboardMixin
+config = app.config
Review comment:
I think this is used for connivence though in general I’m not certain the abstraction/indirection is worth it. I would prefer we use `app.config[...]` on line #62. This is more of a style nit and thus is non-blocking.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9025: [dashboard] fix, add config to optionally enable react replacement fo…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9025: [dashboard] fix, add config to optionally enable react replacement fo…
URL: https://github.com/apache/incubator-superset/pull/9025#issuecomment-578458338
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a0cda321b7d330c2812a512016d579d9bd0c017d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
=======================================
Coverage 59.43% 59.43%
=======================================
Files 369 369
Lines 11743 11743
Branches 2884 2884
=======================================
Hits 6980 6980
Misses 4584 4584
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=footer). Last update [a0cda32...54122a4](https://codecov.io/gh/apache/incubator-superset/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org