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/03/12 06:47:32 UTC
[GitHub] [incubator-superset] asif-ir opened a new pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
asif-ir opened a new pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291
… (#9290)
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### ADDITIONAL INFORMATION
I performed the following steps:
- Renamed the component file from .jsx to .tsx
- No further typedefs were needed
- Replaced the propTypes const with an interface Props definition
- Moved default props into the default arguments within the React component
- Resolved any type errors that may have been exposed by the migration
- Ran tests, coverage and linting and made sure everything was good
+ Added more optional proptypes in Button.jsx, dummy comments in empty functions to remove linting errors. Further linting showed no errors (except pre prevalent warnings)
- Also tested manually from the superset UI. Funtionality remains same for this and all other buttons.
### 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: @graceguo-supercat @nytai @rusackas @kristw
Will migrate more files if this is acceptable.
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391792986
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText: string;
Review comment:
can we make this `selectedText?: string;` and remove the default selectedText?
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391793747
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText: string;
+ stopQuery: () => void;
+ sql: string;
+}
-export default function RunQueryActionButton(props) {
- const runBtnText = props.selectedText
- ? t('Run Selected Query')
- : t('Run Query');
- const btnStyle = props.selectedText ? 'warning' : 'primary';
- const shouldShowStopBtn =
- ['running', 'pending'].indexOf(props.queryState) > -1;
+const RunQueryActionButton = ({
+ allowAsync = false,
+ dbId = null,
+ queryState = '',
+ runQuery = () => {
+ // Do nothing
+ },
+ selectedText = '',
+ stopQuery = () => {
+ // Do nothing
+ },
+ sql = '',
Review comment:
Remove the appropriate defaults re the `Props` comments above
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391792297
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText: string;
+ stopQuery: () => void;
+ sql: string;
+}
-export default function RunQueryActionButton(props) {
- const runBtnText = props.selectedText
- ? t('Run Selected Query')
- : t('Run Query');
- const btnStyle = props.selectedText ? 'warning' : 'primary';
- const shouldShowStopBtn =
- ['running', 'pending'].indexOf(props.queryState) > -1;
+const RunQueryActionButton = ({
+ allowAsync = false,
+ dbId = null,
+ queryState = '',
+ runQuery = () => {
Review comment:
I think `() => undefined` is preferred for a NoOp function. Better yet, you should define a const at the top of the file for a no op function then use it as the defaults here like this:
`const NO_OP = () => undefined;`
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391791542
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText: string;
+ stopQuery: () => void;
+ sql: string;
+}
-export default function RunQueryActionButton(props) {
- const runBtnText = props.selectedText
- ? t('Run Selected Query')
- : t('Run Query');
- const btnStyle = props.selectedText ? 'warning' : 'primary';
- const shouldShowStopBtn =
- ['running', 'pending'].indexOf(props.queryState) > -1;
+const RunQueryActionButton = ({
+ allowAsync = false,
+ dbId = null,
+ queryState = '',
+ runQuery = () => {
+ // Do nothing
+ },
+ selectedText = '',
+ stopQuery = () => {
+ // Do nothing
+ },
+ sql = '',
Review comment:
if the sql sting was required before, we probably don't need a default here
----------------------------------------------------------------
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 merged pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291
----------------------------------------------------------------
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] willbarrett commented on issue #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#issuecomment-598252558
Some relevant reviewers: @rusackas @etr2460 @nytai @suddjian
----------------------------------------------------------------
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] asif-ir commented on issue #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
asif-ir commented on issue #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#issuecomment-598352803
@etr2460: Addressed the above suggestions.
----------------------------------------------------------------
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] asif-ir commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
asif-ir commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391858039
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,55 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+const NO_OP = () => undefined;
+
+interface Props {
+ allowAsync: boolean;
+ dbId?: number;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText?: string;
+ stopQuery: () => void;
+ sql: string;
+}
-export default function RunQueryActionButton(props) {
- const runBtnText = props.selectedText
- ? t('Run Selected Query')
- : t('Run Query');
- const btnStyle = props.selectedText ? 'warning' : 'primary';
- const shouldShowStopBtn =
- ['running', 'pending'].indexOf(props.queryState) > -1;
+const RunQueryActionButton = ({
+ allowAsync = false,
+ dbId,
+ queryState = '',
+ runQuery = NO_OP,
+ selectedText,
+ stopQuery = NO_OP,
+ sql,
+}: Props) => {
+ const runBtnText = selectedText ? t('Run Selected Query') : t('Run Query');
+ const btnStyle = selectedText ? 'warning' : 'primary';
+ const shouldShowStopBtn = ['running', 'pending'].indexOf(queryState) > -1;
Review comment:
Done.
----------------------------------------------------------------
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] asif-ir commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
asif-ir commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391814478
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
Review comment:
Didn't change this one since queryState is being using in _indexOf_ few lines below which gets type-angry if you feed *undefined* to it.
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391793539
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
+ queryState: string;
Review comment:
can we make this `queryState?: string` and remove the default queryState?
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391851343
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,55 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+const NO_OP = () => undefined;
+
+interface Props {
+ allowAsync: boolean;
+ dbId?: number;
+ queryState: string;
+ runQuery: (c?: boolean) => void;
+ selectedText?: string;
+ stopQuery: () => void;
+ sql: string;
+}
-export default function RunQueryActionButton(props) {
- const runBtnText = props.selectedText
- ? t('Run Selected Query')
- : t('Run Query');
- const btnStyle = props.selectedText ? 'warning' : 'primary';
- const shouldShowStopBtn =
- ['running', 'pending'].indexOf(props.queryState) > -1;
+const RunQueryActionButton = ({
+ allowAsync = false,
+ dbId,
+ queryState = '',
+ runQuery = NO_OP,
+ selectedText,
+ stopQuery = NO_OP,
+ sql,
+}: Props) => {
+ const runBtnText = selectedText ? t('Run Selected Query') : t('Run Query');
+ const btnStyle = selectedText ? 'warning' : 'primary';
+ const shouldShowStopBtn = ['running', 'pending'].indexOf(queryState) > -1;
Review comment:
you can change this to `const shouldShowStopBtn = !!queryState && ['running', 'pending'].indexOf(queryState) > -1
`
Then you won't need to default queryState to empty string
----------------------------------------------------------------
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 #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9291: [SIP-36] Migrate RunQueryActionButton.jsx to RunQueryActionButton.tsx…
URL: https://github.com/apache/incubator-superset/pull/9291#discussion_r391792955
##########
File path: superset-frontend/src/SqlLab/components/RunQueryActionButton.tsx
##########
@@ -17,53 +17,57 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { t } from '@superset-ui/translation';
import Button from '../../components/Button';
-const propTypes = {
- allowAsync: PropTypes.bool.isRequired,
- dbId: PropTypes.number,
- queryState: PropTypes.string,
- runQuery: PropTypes.func.isRequired,
- selectedText: PropTypes.string,
- stopQuery: PropTypes.func.isRequired,
- sql: PropTypes.string.isRequired,
-};
-const defaultProps = {
- allowAsync: false,
- sql: '',
-};
+interface Props {
+ allowAsync: boolean;
+ dbId: number | null;
Review comment:
can we make this `dbId?: number` and remove the default dbId?
----------------------------------------------------------------
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