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 2022/01/10 20:53:12 UTC
[GitHub] [superset] jcahela opened a new pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
jcahela opened a new pull request #17987:
URL: https://github.com/apache/superset/pull/17987
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
I converted the ExploreResultsButton from class component to functional component.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi closed pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi closed pull request #17987:
URL: https://github.com/apache/superset/pull/17987
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17987:
URL: https://github.com/apache/superset/pull/17987#discussion_r781644803
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
Review comment:
Please convert all of your functions to arrow functions (I'll mark the others with 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #17987:
URL: https://github.com/apache/superset/pull/17987#discussion_r782248631
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -40,16 +40,8 @@ const defaultProps = {
query: {},
};
-class ExploreResultsButton extends React.PureComponent {
- constructor(props) {
- super(props);
- this.getInvalidColumns = this.getInvalidColumns.bind(this);
- this.renderInvalidColumnMessage =
- this.renderInvalidColumnMessage.bind(this);
- }
-
- getColumns() {
- const { props } = this;
+function ExploreResultsButton(props) {
Review comment:
All the others I completely agree with, but I think that this, being the functional component itself, does not need to be an arrow function.
You need to destructure props here, you can do that by console logging the props and writing them out individually, its more explicit.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #17987:
URL: https://github.com/apache/superset/pull/17987#discussion_r782248631
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -40,16 +40,8 @@ const defaultProps = {
query: {},
};
-class ExploreResultsButton extends React.PureComponent {
- constructor(props) {
- super(props);
- this.getInvalidColumns = this.getInvalidColumns.bind(this);
- this.renderInvalidColumnMessage =
- this.renderInvalidColumnMessage.bind(this);
- }
-
- getColumns() {
- const { props } = this;
+function ExploreResultsButton(props) {
Review comment:
All the others I completely agree with, but I think that this, being the functional component itself, does not need to be an arrow function.
However, I think that you need to destructure props here, you can do that by console logging the props and writing them out individually, its more explicit.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17987:
URL: https://github.com/apache/superset/pull/17987#discussion_r781648026
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -40,16 +40,8 @@ const defaultProps = {
query: {},
};
-class ExploreResultsButton extends React.PureComponent {
- constructor(props) {
- super(props);
- this.getInvalidColumns = this.getInvalidColumns.bind(this);
- this.renderInvalidColumnMessage =
- this.renderInvalidColumnMessage.bind(this);
- }
-
- getColumns() {
- const { props } = this;
+function ExploreResultsButton(props) {
Review comment:
Please convert this to an arrow function.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lyndsiWilliams commented on a change in pull request #17987: refactor: convert exploreresultsbutton from class component to functional component
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #17987:
URL: https://github.com/apache/superset/pull/17987#discussion_r781645036
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
return moment
- .duration(this.props.query.endDttm - this.props.query.startDttm)
+ .duration(props.query.endDttm - props.query.startDttm)
.asSeconds();
}
- getInvalidColumns() {
+ function getInvalidColumns() {
const re1 = /__\d+$/; // duplicate column name pattern
const re2 = /^__timestamp/i; // reserved temporal column alias
-
- return this.props.query.results.selected_columns
+ return props.query.results.selected_columns
.map(col => col.name)
.filter(col => re1.test(col) || re2.test(col));
}
- datasourceName() {
- const { query } = this.props;
+ function datasourceName() {
Review comment:
Please convert this to an arrow function.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
return moment
- .duration(this.props.query.endDttm - this.props.query.startDttm)
+ .duration(props.query.endDttm - props.query.startDttm)
.asSeconds();
}
- getInvalidColumns() {
+ function getInvalidColumns() {
Review comment:
Please convert this to an arrow function.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
Review comment:
Please convert this to an arrow function.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
return moment
- .duration(this.props.query.endDttm - this.props.query.startDttm)
+ .duration(props.query.endDttm - props.query.startDttm)
.asSeconds();
}
- getInvalidColumns() {
+ function getInvalidColumns() {
const re1 = /__\d+$/; // duplicate column name pattern
const re2 = /^__timestamp/i; // reserved temporal column alias
-
- return this.props.query.results.selected_columns
+ return props.query.results.selected_columns
.map(col => col.name)
.filter(col => re1.test(col) || re2.test(col));
}
- datasourceName() {
- const { query } = this.props;
+ function datasourceName() {
const uniqueId = shortid.generate();
let datasourceName = uniqueId;
- if (query) {
- datasourceName = query.user ? `${query.user}-` : '';
- datasourceName += `${query.tab}-${uniqueId}`;
+ if (props.query) {
+ datasourceName = props.query.user ? `${props.query.user}-` : '';
+ datasourceName += `${props.query.tab}-${uniqueId}`;
}
return datasourceName;
}
- buildVizOptions() {
- const { schema, sql, dbId, templateParams } = this.props.query;
+ function buildVizOptions() {
+ const { schema, sql, dbId, templateParams } = props.query;
return {
dbId,
schema,
sql,
templateParams,
- datasourceName: this.datasourceName(),
- columns: this.getColumns(),
+ datasourceName: datasourceName(),
+ columns: getColumns(),
};
}
- renderTimeoutWarning() {
+ function renderTimeoutWarning() {
Review comment:
Please convert this to an arrow function.
Also - GH actions says that this function is defined but never used. If you come across any unused functions like this, please remove them.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -60,57 +52,55 @@ class ExploreResultsButton extends React.PureComponent {
return [];
}
- getQueryDuration() {
+ function getQueryDuration() {
return moment
- .duration(this.props.query.endDttm - this.props.query.startDttm)
+ .duration(props.query.endDttm - props.query.startDttm)
.asSeconds();
}
- getInvalidColumns() {
+ function getInvalidColumns() {
const re1 = /__\d+$/; // duplicate column name pattern
const re2 = /^__timestamp/i; // reserved temporal column alias
-
- return this.props.query.results.selected_columns
+ return props.query.results.selected_columns
.map(col => col.name)
.filter(col => re1.test(col) || re2.test(col));
}
- datasourceName() {
- const { query } = this.props;
+ function datasourceName() {
const uniqueId = shortid.generate();
let datasourceName = uniqueId;
- if (query) {
- datasourceName = query.user ? `${query.user}-` : '';
- datasourceName += `${query.tab}-${uniqueId}`;
+ if (props.query) {
+ datasourceName = props.query.user ? `${props.query.user}-` : '';
+ datasourceName += `${props.query.tab}-${uniqueId}`;
}
return datasourceName;
}
- buildVizOptions() {
- const { schema, sql, dbId, templateParams } = this.props.query;
+ function buildVizOptions() {
Review comment:
Please convert this to an arrow function.
Also - GH actions says that this function is defined but never used. If you come across any unused functions like this, please remove them.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -149,28 +139,29 @@ class ExploreResultsButton extends React.PureComponent {
);
}
- render() {
- const allowsSubquery =
- this.props.database && this.props.database.allows_subquery;
- return (
- <>
- <Button
- buttonSize="small"
- onClick={this.props.onClick}
- disabled={!allowsSubquery}
- tooltip={t('Explore the result set in the data exploration view')}
- >
- <InfoTooltipWithTrigger
- icon="line-chart"
- placement="top"
- label="explore"
- />{' '}
- {t('Explore')}
- </Button>
- </>
- );
- }
+ const allowsSubquery = props.database && props.database.allows_subquery;
+
+ return (
+ <>
+ <Button
+ buttonSize="small"
+ onClick={props.onClick}
+ disabled={!allowsSubquery}
+ tooltip={t(
+ 'Explore the result set in the data exploration viewwwwwwwwwwwwww',
+ )}
+ >
+ <InfoTooltipWithTrigger
+ icon="line-chart"
+ placement="top"
+ label="explore"
+ />{' '}
+ {t('Jason')}
Review comment:
```suggestion
{t('Explore')}
```
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -149,28 +139,29 @@ class ExploreResultsButton extends React.PureComponent {
);
}
- render() {
- const allowsSubquery =
- this.props.database && this.props.database.allows_subquery;
- return (
- <>
- <Button
- buttonSize="small"
- onClick={this.props.onClick}
- disabled={!allowsSubquery}
- tooltip={t('Explore the result set in the data exploration view')}
- >
- <InfoTooltipWithTrigger
- icon="line-chart"
- placement="top"
- label="explore"
- />{' '}
- {t('Explore')}
- </Button>
- </>
- );
- }
+ const allowsSubquery = props.database && props.database.allows_subquery;
+
+ return (
+ <>
+ <Button
+ buttonSize="small"
+ onClick={props.onClick}
+ disabled={!allowsSubquery}
+ tooltip={t(
+ 'Explore the result set in the data exploration viewwwwwwwwwwwwww',
Review comment:
```suggestion
'Explore the result set in the data exploration view',
```
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -40,16 +40,8 @@ const defaultProps = {
query: {},
};
-class ExploreResultsButton extends React.PureComponent {
- constructor(props) {
- super(props);
- this.getInvalidColumns = this.getInvalidColumns.bind(this);
- this.renderInvalidColumnMessage =
- this.renderInvalidColumnMessage.bind(this);
- }
-
- getColumns() {
- const { props } = this;
+function ExploreResultsButton(props) {
Review comment:
Please convert all of your functions to arrow functions (I'll mark the others with comments)
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -149,28 +139,29 @@ class ExploreResultsButton extends React.PureComponent {
);
}
- render() {
- const allowsSubquery =
- this.props.database && this.props.database.allows_subquery;
- return (
- <>
- <Button
- buttonSize="small"
- onClick={this.props.onClick}
- disabled={!allowsSubquery}
- tooltip={t('Explore the result set in the data exploration view')}
- >
- <InfoTooltipWithTrigger
- icon="line-chart"
- placement="top"
- label="explore"
- />{' '}
- {t('Explore')}
- </Button>
- </>
- );
- }
+ const allowsSubquery = props.database && props.database.allows_subquery;
Review comment:
```suggestion
const allowsSubquery = props.database?.allows_subquery;
```
Nit: This code can be cleaned with [optional chaining](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining).
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -40,16 +40,8 @@ const defaultProps = {
query: {},
};
-class ExploreResultsButton extends React.PureComponent {
- constructor(props) {
- super(props);
- this.getInvalidColumns = this.getInvalidColumns.bind(this);
- this.renderInvalidColumnMessage =
- this.renderInvalidColumnMessage.bind(this);
- }
-
- getColumns() {
- const { props } = this;
+function ExploreResultsButton(props) {
+ function getColumns() {
Review comment:
Please convert this to an arrow function.
##########
File path: superset-frontend/src/SqlLab/components/ExploreResultsButton/index.jsx
##########
@@ -129,8 +119,8 @@ class ExploreResultsButton extends React.PureComponent {
);
}
- renderInvalidColumnMessage() {
- const invalidColumns = this.getInvalidColumns();
+ function renderInvalidColumnMessage() {
Review comment:
Please convert this to an arrow function.
Also - GH actions says that this function is defined but never used. If you come across any unused functions like this, please remove them.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org