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

build: add workflow preferring TypeScript files #9901

Merged
merged 21 commits into from
Jun 1, 2020

Conversation

etr2460
Copy link
Member

@etr2460 etr2460 commented May 25, 2020

SUMMARY

Adds a Github workflow to check if any .js or .jsx files were added in a PR, and adds a comment to the PR asking to change the files to use TypeScript. This check always passes and will never prevent merge.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Comments when a .js file is added:
Screen Shot 2020-05-24 at 10 25 17 PM

Skips the comment step when no js files are added:
Screen Shot 2020-05-24 at 10 27 58 PM

TEST PLAN

Add a .js file to this PR and see the message appear. See the comment step skip when no js is added. Also test the jq command locally to ensure it works will all file extensions

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

to: @ktmud @john-bodley @kristw @nytai @graceguo-supercat

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2020

Codecov Report

Merging #9901 into master will increase coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9901      +/-   ##
==========================================
+ Coverage   71.22%   71.67%   +0.44%     
==========================================
  Files         585      585              
  Lines       30828    32789    +1961     
  Branches     3237     3237              
==========================================
+ Hits        21957    23501    +1544     
- Misses       8762     9179     +417     
  Partials      109      109              
Flag Coverage Δ
#cypress 53.76% <ø> (-0.08%) ⬇️
#javascript 59.38% <ø> (ø)
#python 72.13% <ø> (+0.73%) ⬆️
Impacted Files Coverage Δ
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx 65.90% <0.00%> (-6.82%) ⬇️
superset/jinja_context.py 84.00% <0.00%> (-1.72%) ⬇️
.../src/dashboard/components/gridComponents/Chart.jsx 86.51% <0.00%> (-1.13%) ⬇️
superset/errors.py 100.00% <0.00%> (ø)
superset/typing.py 100.00% <0.00%> (ø)
superset/viz_sip38.py 0.00% <0.00%> (ø)
superset/utils/import_datasource.py 100.00% <0.00%> (ø)
superset/sql_parse.py 99.30% <0.00%> (+<0.01%) ⬆️
superset/db_engine_specs/base.py 87.24% <0.00%> (+0.03%) ⬆️
superset/connectors/base/models.py 90.58% <0.00%> (+0.12%) ⬆️
... and 9 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 9edfc8f...4463234. Read the comment docs.

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 25, 2020
@pull-request-size pull-request-size bot added size/S and removed size/M labels May 25, 2020
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 25, 2020
@pull-request-size pull-request-size bot added size/S and removed size/M labels May 25, 2020
@apache apache deleted a comment from github-actions bot May 25, 2020
@apache apache deleted a comment from github-actions bot May 25, 2020
@github-actions
Copy link
Contributor

It looks like your PR contains new .js or .jsx files. As decided in SIP-36, all new files should be written in TypeScript. Please convert new JavaScript files to TypeScript and then re-request review.

@etr2460 etr2460 marked this pull request as ready for review May 25, 2020 05:29
@etr2460 etr2460 changed the title build: add workflow preventing non TypeScript Files build: add workflow preferring TypeScript files May 25, 2020
@ktmud
Copy link
Member

ktmud commented May 25, 2020

This is a great idea!

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
msg: "It looks like your PR contains new `.js` or `.jsx` files. As decided in [SIP-36](https://github.com/apache/incubator-superset/issues/9101), all new files should be written in TypeScript. Please convert new JavaScript files to TypeScript and then re-request review."
Copy link
Member

@ktmud ktmud May 25, 2020

Choose a reason for hiding this comment

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

How about a title to make this more visible

        with:
          msg: |
            ## WARNING: TypeScript Preferred

            It looks like your PR contains new `.js` or `.jsx` files. As decided in [SIP-36](https://github.com/apache/incubator-superset/issues/9101), all new files should be written in TypeScript. Please convert new JavaScript files to TypeScript (`.ts` or `tsx`) then re-request review.

WARNING: Prefer TypeScript

It looks like your PR contains new .js or .jsx files. As decided in SIP-36, all new files should be written in TypeScript. Please convert new JavaScript files to TypeScript then re-request review.


It'd also be nice if the comment could be deleted when new js was removed from PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, I'll add a title.

I couldn't find a github action for deleting comments unfortunately, so I don't think there's an easy way to implement your second comment...

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 26, 2020
@etr2460
Copy link
Member Author

etr2460 commented May 27, 2020

ping @ktmud for rereview

@etr2460 etr2460 merged commit c7618ee into master Jun 1, 2020
@etr2460 etr2460 deleted the erik-ritter--prevent-non-typescript-files branch July 1, 2020 15:14
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Add workflow preventing non TypeScript Files

* Create comment_preferring_typescript.md

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Delete comment_preferring_typescript.md

* Create comment_preferring_typescript.md

* Delete comment_preferring_typescript.md

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Update prevent_non_typescript_files.yml

* Create test.js

* Delete test.js

* Update prevent_non_typescript_files.yml

* Rename prevent_non_typescript_files.yml to prefer_typescript.yml

* Update prefer_typescript.yml
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.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/M 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants