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.

Sign up for GitHub

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

Jump to bottom

ESLint: Re-enable rule default-props-match-prop-types #10868

Merged
merged 4 commits into from
Sep 22, 2020
Merged

ESLint: Re-enable rule default-props-match-prop-types #10868

merged 4 commits into from
Sep 22, 2020

Conversation

kgabryje
Copy link
Member

SUMMARY

Re-enable ESLint rule default-props-match-prop-types, which was disabled in PR #10839. Code was refactored to fix the errors raised by the rule.

TEST PLAN

Run npm run lint, verify that there are no new Javascript/Typescript errors.

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

@rusackas rusackas requested a review from etr2460 September 14, 2020 17:56
mistercrunch
mistercrunch requested changes Sep 15, 2020
View reviewed changes
superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx
@@ -30,7 +30,6 @@ const propTypes = {

const defaultProps = {
colorScheme: undefined,
onChange: () => {},
Copy link
Member

Choose a reason for hiding this comment

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

this one is making me nervous, can we remove the .isRequired instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

superset-frontend/src/dashboard/components/DashboardBuilder.jsx
@@ -64,7 +64,6 @@ const propTypes = {
};

const defaultProps = {
showBuilderPane: false,
Copy link
Member

Choose a reason for hiding this comment

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

seems less risky to remove the .isRequired here too (unless you can confirm you checked all the calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did, but you're right, let's keep the default and remove isRequired

Copy link
Member

Choose a reason for hiding this comment

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

I was confused about the type here and tracked it into the child component, and it looks unused there. Can you double check and remove all references to it here if that's the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that not only showBuilderPane was unused, but also colorScheme and setColorSchemeAndUnsavedChanges. I removed references from this file, containers/DashboardBuilder and the spec file.

superset-frontend/src/dashboard/components/SliceAdder.jsx Outdated
@@ -45,7 +45,6 @@ const propTypes = {
};

const defaultProps = {
selectedSliceIds: [],
Copy link
Member

Choose a reason for hiding this comment

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

let's remove .isRequired here to (unless you can confirm you checked all the calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did, but you're right, let's keep the default and remove isRequired

rusackas
rusackas approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM, but will leave merging to @mistercrunch to make sure he's happy with the changes made :)

@kgabryje
Copy link
Member Author

@mistercrunch Can we merge it now? I made the changes that you asked for

@rusackas rusackas merged commit 9f01a7f into apache:master Sep 22, 2020
graceguo-supercat pushed a commit to airbnb/superset-fork that referenced this pull request Sep 24, 2020
revert apache#10868 to fix dashboard tab switch
e72e71e
@graceguo-supercat
Copy link

@kgabryje we just found this PR broke Dashboard tab switch:
mSqaIe8eS4

i can't find exactly which line of change is wrong. But since dashboard tab switch is very critical function, i will have to revert this PR.

@kgabryje
Copy link
Member Author

@graceguo-supercat I'm sorry to hear that! I'll try to look into it

graceguo-supercat pushed a commit that referenced this pull request Sep 24, 2020
Revert "ESLint: Re-enable rule default-props-match-prop-types (#10868)"
c3b6d4f 
This reverts commit 9f01a7f.
@graceguo-supercat graceguo-supercat mentioned this pull request Sep 24, 2020
Merged
@junlincc junlincc mentioned this pull request Sep 24, 2020
Closed
etr2460 pushed a commit that referenced this pull request Sep 24, 2020
Revert "ESLint: Re-enable rule default-props-match-prop-types (#10868)…
f99753a 
…" (#11050)

This reverts commit 9f01a7f.
@kgabryje kgabryje mentioned this pull request Sep 25, 2020
Merged
6 tasks
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 27, 2020
Merge remote-tracking branch 'upstream/master' into feat/add_all_dash…
63c3295 
…boards_permissions

* upstream/master: (46 commits)
  fix: surface connection error messages on the client (apache#11077)
  fix(jest): using UTC mock date (apache#11079)
  removing unused component (apache#11072)
  changing to the correct hex color (apache#11073)
  style: remove unecessary padding (apache#11071)
  fix: database list checkboxes (apache#11068)
  feat: adding all icons from the design system to the codebase (apache#11033)
  fix: sql lab autocomplete width (apache#11063)
  clickable labels have outlines, storybook shows them (apache#11034)
  fixed routes for customer in docs (apache#11052)
  Revert "style: fix checkbox color (apache#10970)" (apache#11051)
  feat: add "created by" to dashboard CRUD view (apache#11030)
  Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (apache#11037)
  chore: updated lint rules in models module (apache#11036)
  Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (apache#11038)
  Enabled argument-differ for bulk_delete (apache#11039)
  Enabled no-self-use pylint rule in security. Formatter (apache#11041)
  Changed variable name from capitals to lowercase and changed lint rule (apache#11044)
  Revert "ESLint: Re-enable rule default-props-match-prop-types (apache#10868)" (apache#11050)
  feat(saved_queries): add custom api filter for all string & text fields (apache#11031)
  ...

# Conflicts:
#	superset/config.py
#	tests/dashboards/api_tests.py
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 27, 2020
Merge remote-tracking branch 'upstream/master' into feat/add_all_dash…
1ee44c2 
…boards_permissions

* upstream/master: (46 commits)
  fix: surface connection error messages on the client (apache#11077)
  fix(jest): using UTC mock date (apache#11079)
  removing unused component (apache#11072)
  changing to the correct hex color (apache#11073)
  style: remove unecessary padding (apache#11071)
  fix: database list checkboxes (apache#11068)
  feat: adding all icons from the design system to the codebase (apache#11033)
  fix: sql lab autocomplete width (apache#11063)
  clickable labels have outlines, storybook shows them (apache#11034)
  fixed routes for customer in docs (apache#11052)
  Revert "style: fix checkbox color (apache#10970)" (apache#11051)
  feat: add "created by" to dashboard CRUD view (apache#11030)
  Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (apache#11037)
  chore: updated lint rules in models module (apache#11036)
  Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (apache#11038)
  Enabled argument-differ for bulk_delete (apache#11039)
  Enabled no-self-use pylint rule in security. Formatter (apache#11041)
  Changed variable name from capitals to lowercase and changed lint rule (apache#11044)
  Revert "ESLint: Re-enable rule default-props-match-prop-types (apache#10868)" (apache#11050)
  feat(saved_queries): add custom api filter for all string & text fields (apache#11031)
  ...

# Conflicts:
#	superset/config.py
#	tests/dashboards/api_tests.py
rorymillersoft referenced this pull request in nets-aric/incubator-superset Oct 28, 2020
@rorymillersoft
Merge commit 'c14a06db4185b9b043116708b649b0be2ad17a1f' into superset…
1ac9518 
…_additions

* commit 'c14a06db4185b9b043116708b649b0be2ad17a1f':
  Revert "style: fix checkbox color (#10970)"
  feat: add "created by" to dashboard CRUD view (#11030)
  Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (#11037)
  chore: updated lint rules in models module (#11036)
  Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (#11038)
  Enabled argument-differ for bulk_delete (#11039)
  Enabled no-self-use pylint rule in security. Formatter (#11041)
  Changed variable name from capitals to lowercase and changed lint rule (#11044)
  Revert "ESLint: Re-enable rule default-props-match-prop-types (#10868)" (#11050)
  feat(saved_queries): add custom api filter for all string & text fields (#11031)
  Support jinja templates (#11008)
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@kgabryje @auxten
ESLint: Re-enable rule default-props-match-prop-types (apache#10868)
c9ad002 
* Re-enable rule default-props-match-prop-types

* Restore default props and remove isRequired

* Remove unused props
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@auxten
Revert "ESLint: Re-enable rule default-props-match-prop-types (apache…
8fc6957 
…#10868)" (apache#11050)

This reverts commit 9f01a7f.
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Revert "ESLint: Re-enable rule default-props-match-prop-types (apache…
c82df45 
…#10868)"

This reverts commit 27db55d.
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch requested changes

@rusackas rusackas rusackas approved these changes

@etr2460 etr2460 Awaiting requested review from etr2460

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.38.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kgabryje @graceguo-supercat @mistercrunch @rusackas