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

chore: standardize npm script names #461

Merged
merged 2 commits into from Aug 14, 2023

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Jul 29, 2023

Prerequisites checklist

What is the purpose of this pull request?

Refers eslint/eslint#14827

What changes did you make? (Give an overview)

This updates the names of the scripts in package.json to be consistent with the new standard.

Is there anything you'd like reviewers to focus on?

Is there any script we need to update on Netlify?

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for zh-hans-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/zh-hans-eslint/deploys/64d09d99c47d820008e90e8d
😎 Deploy Preview https://deploy-preview-461--zh-hans-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for es-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/es-eslint/deploys/64d09d9981026d00088da78b
😎 Deploy Preview https://deploy-preview-461--es-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for new-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/new-eslint/deploys/64d09d9992156a0008ede044
😎 Deploy Preview https://deploy-preview-461--new-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for hi-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/hi-eslint/deploys/64d09d99a455c200081b1505
😎 Deploy Preview https://deploy-preview-461--hi-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for fr-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/fr-eslint/deploys/64d09d99d400c50008a6b794
😎 Deploy Preview https://deploy-preview-461--fr-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for ja-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/ja-eslint/deploys/64d09d99916e590008b241bc
😎 Deploy Preview https://deploy-preview-461--ja-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for de-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/de-eslint/deploys/64d09d997cccd10008a4a639
😎 Deploy Preview https://deploy-preview-461--de-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 29, 2023

Deploy Preview for pt-br-eslint ready!

Name Link
🔨 Latest commit 0102554
🔍 Latest deploy log https://app.netlify.com/sites/pt-br-eslint/deploys/64d09d99bd0ae20008783e5a
😎 Deploy Preview https://deploy-preview-461--pt-br-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

amareshsm
amareshsm previously approved these changes Jul 29, 2023
Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

LGTM. Leaving this open for 2nd review.

Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

It seems that no changes should be made to the indent style.

The .editorconfig is used in eslint/eslint to apply 4 space indentation to all files. I think the eslint.org repository should be consistent as well, plus we could create a PR to add the missing .editorconfig file to it.

@harish-sethuraman harish-sethuraman mentioned this pull request Aug 1, 2023
1 task
@harish-sethuraman
Copy link
Member

@snitin315 can you fix the spacing so that we can review the PR?

@snitin315
Copy link
Contributor Author

Yeah, my editor automatically formatted the indentation.

"build:sass:watch": "sass --watch src/assets/scss:src/assets/css",
"build:website": "npx @11ty/eleventy",
"build:website:watch": "cross-env IS_DEV=true eleventy --serve --port=2022",
"fetch": "npm-run-all --parallel fetch:*",
Copy link
Member

Choose a reason for hiding this comment

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

according to https://github.com/eslint/eslint/blob/main/docs/src/contribute/package-json-conventions.md we do not have any convention for fetch. May be we should consider adding it to the doc or replace these fetch with something like build:fetch?

Copy link
Member

Choose a reason for hiding this comment

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

build:fetch looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

@mdjermanovic what do you think? I could live with adding a fetch: prefix.

Otherwise, maybe build:remote-data?

Copy link
Member

@kecrily kecrily Aug 7, 2023

Choose a reason for hiding this comment

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

If it can just add the fetch prefix, then that's better in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Adding new fetch: prefix sounds good to me.

build: = generates a set of files from local files only.
fetch: = Like build:, but includes fetching some data from remote locations.

Copy link
Member

Choose a reason for hiding this comment

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

Aligned on introducing fetch: scripts. Lets add it to the contribute's package-json-convention also @snitin315

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the docs.

Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

LGTM, Leaving this open for 2nd review.

Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

@harish-sethuraman harish-sethuraman merged commit 36091ea into main Aug 14, 2023
45 checks passed
@harish-sethuraman harish-sethuraman deleted the chore/npm-scripts-standardize branch August 14, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

6 participants