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

Remove the lint script from blockly-scripts #1976

Closed
1 task done
maribethb opened this issue Sep 26, 2023 · 7 comments · Fixed by #2044
Closed
1 task done

Remove the lint script from blockly-scripts #1976

maribethb opened this issue Sep 26, 2023 · 7 comments · Fixed by #2044
Assignees
Labels
good first issue Good for newcomers type: feature request New feature or request

Comments

@maribethb
Copy link
Contributor

maribethb commented Sep 26, 2023

Check for duplicates

  • I have searched for similar issues before opening a new one.

Component

dev-scripts

Problem

The lint script in dev-scripts runs eslint through the JS API. After #1946 is merged, we'll no longer need this script because eslint can be invoked directly on the command line from any level of blockly-samples. This is an improvement because the eslint cli has better formatted output (especially when run at the entire repository level) and the flat config is easier to use and make changes to.

Request

Remove the lint script from dev-scripts. This is a breaking change for the plugin. The lint script already logs a deprecation warning when run. I hope that nobody is using this script outside of blockly-samples.

Alternatives considered

No response

Additional context

If you are currently using blockly-scripts lint, switch to invoking eslint through its cli or write your own script that uses the eslint js api. Also, it would help us if you commented on this issue with your use case.

@maribethb maribethb added type: feature request New feature or request triage labels Sep 26, 2023
@maribethb maribethb added good first issue Good for newcomers and removed triage labels Sep 27, 2023
@shivalipatel6
Copy link
Contributor

Hi I would like to be assigned to this issue! This would be my second contribution to Blockly-samples, my first being through the Grace Hopper Conference.

@BeksOmega
Copy link
Contributor

Hello @shivalipatel6 Thanks for your interest and coming back to the project :D I can assign you to the issue, but just so you know we'll have to hold your fix until after #1946 is merged.

@shivalipatel6
Copy link
Contributor

shivalipatel6 commented Oct 11, 2023

@BeksOmega Okay! I'll wait until #1946 is merged! I noticed that most of the good first issue projects were connected to that so I'm willing to wait. Just to make sure the fix is asking me to delete lint.js script after the #1946 issue is merged?

@BeksOmega
Copy link
Contributor

Just to make sure the fix is asking me to delete lint.js script after the #1946 issue is merged?

Hello again :D It is asking you to:

  1. Do that - delete the lint.js script.
  2. Delete any references to the lint script. So anywhere it says "lint": "blockly-scripts lint", in the package.json of a plugin.

@maribethb
Copy link
Contributor Author

Hi @shivalipatel6, one small correction to what Beka said. You won't need to touch the package.json files of every plugin, because I will have already updated them to use the new eslint config. You will need to delete the lint.js script, remove it from the "available scripts" in dev-scripts/bin/blockly-scripts.js, and remove it from the README of dev-scripts. That should be it!

shivalipatel6 added a commit to shivalipatel6/blockly-samples that referenced this issue Oct 28, 2023
Remove the lint script from blockly-scripts
google#1976
@shivalipatel6
Copy link
Contributor

I have a question:
I just noticed that https://developers.google.com/blockly/guides/contribute/samples#making_and_verifying_a_change tells users learning how to verify a change that you need to run npm run lint, is that a problem now that I have deleted the lint.js script? I think that because eslint will be able to be run from the command line from any level of blockly samples that this should be fine, but I am trying to expand my understanding of blockly-samples so I figured I should just ask.

@maribethb
Copy link
Contributor Author

Hi @shivalipatel6, this won't be a problem as each plugin still contains a script for linting so that you can run npm run lint from within each plugin (or from the root of blockly-samples). The script used to be blockly-scripts lint which would run the script you deleted in your PR. Now, the script is eslint . which just runs the eslint command line directly. You can find this information in the package.json for any plugin.

I hope that helps! I'll be reviewing your PR later today. Let me know if you have any other questions. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants