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.

Sign up for GitHub

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

Jump to bottom

Scripts: Add default file patterns for linting scripts #15890

Merged
merged 2 commits into from
Jun 5, 2019
Merged

Scripts: Add default file patterns for linting scripts #15890

merged 2 commits into from
Jun 5, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 29, 2019 •
edited
Loading

Description

This PR adds default patterns to match files for linting scripts in @wordpress/scripts package. See updated docs for more details:

The rationale behind those changes is that everything should work out of the box and plugin developer shouldn't be concerned about file patterns when using those commands. It's still possible to override them but for the majority of cases, it should work seamlessly.

How has this been tested?

I ensured all commands work as expected with Gutenberg:

  • npm run lint-js
  • npm run lint-js:fix
  • npx wp-scripts lint-js packages/e2e-tests/specs/plugins/ - you should see less warnings than when running npm run lint-js
  • npm run lint-css
  • npm run lint-css:fix
  • npx wp-scripts lint-style "vendor/" - you should see tons of errors :)
  • npm run lint-pkg-json
  • npx wp-scripts lint-pkg-json ./packages - you should see errrors in the root package.json file

You can also introduce errors yourself and check if they are detected.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

gziolo
gziolo commented May 30, 2019
View reviewed changes
packages/scripts/scripts/lint-js.js
hasPackageProp,
hasProjectFile,
} = require( '../utils' );

const args = getCliArgs();

const defaultFilesArgs = ! hasFileInCliArgs ? [ '.' ] : [];
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we use ./src instead of . to align with wp-scripts build?

Copy link
Member Author

Choose a reason for hiding this comment

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

In #15977, I propose we ignore build and node_modules folders by default, so this should be less important than at the moment.

gziolo
gziolo commented May 30, 2019
View reviewed changes
packages/scripts/scripts/lint-style.js
hasProjectFile,
hasPackageProp,
} = require( '../utils' );

const args = getCliArgs();

const defaultFilesArgs = ! hasFileInCliArgs ? [ '**/*.{css,scss}' ] : [];
Copy link
Member Author

Choose a reason for hiding this comment

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

**/*.{css,scss} - I'm not convinced about this proposal so I'm happy to revisit based on the feedback.

I also consider myself: src/**/*.{css,scss} to be ready for integration with wp-scripts build. We can always revisit later once we have #14801 implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

In #15977, I propose we ignore build and node_modules folders by default, so this should be less important than at the moment.

@gziolo gziolo added the [Package] Scripts /packages/scripts label May 30, 2019
@gziolo gziolo self-assigned this May 30, 2019
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label May 30, 2019
@gziolo gziolo marked this pull request as ready for review May 30, 2019 13:09
@gziolo gziolo mentioned this pull request Jun 4, 2019
Merged
@gziolo gziolo requested a review from iandunn June 4, 2019 09:11
ntwb
ntwb reviewed Jun 4, 2019
View reviewed changes
packages/scripts/README.md
@@ -114,14 +114,18 @@ _Example:_
```json
{
"scripts": {
"lint:js": "wp-scripts lint-js ."
"lint:js": "wp-scripts lint-js",
"lint:js:src": "wp-scripts lint-js ./src"
Copy link
Member

Choose a reason for hiding this comment

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

I like this, it ties in with what you mentioned here: #15890 (comment)

ntwb
ntwb approved these changes Jun 4, 2019
View reviewed changes
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@gziolo gziolo added this to the 5.9 (Gutenberg) milestone Jun 5, 2019
@gziolo gziolo merged commit 941c6ec into master Jun 5, 2019
@gziolo gziolo deleted the update/scripts-defaults branch June 5, 2019 11:07
@aduth
Copy link
Member

aduth commented Jun 10, 2019 •
edited
Loading

This seems to have broken the lint-js script. Running npm run lint-js outputs ESLint's default usage instructions.

> gutenberg@5.9.0-rc.1 lint-js /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg
> wp-scripts lint-js

eslint [options] file.js [file.js] [dir]

Basic configuration:
  --no-eslintrc                Disable use of configuration from .eslintrc.*
  -c, --config path::String    Use this configuration, overriding .eslintrc.* config options if present
...

See build: https://travis-ci.com/WordPress/gutenberg/jobs/205608992
Compare to prior commit: https://travis-ci.com/WordPress/gutenberg/jobs/205604963

aduth
aduth reviewed Jun 10, 2019
View reviewed changes
packages/scripts/scripts/lint-pkg-json.js
@@ -38,7 +41,7 @@ const defaultIgnoreArgs = ! hasIgnoredFiles ?

const result = spawn(
resolveBin( 'npm-package-json-lint', { executable: 'npmPkgJsonLint' } ),
[ ...config, ...defaultIgnoreArgs, ...args ],
[ ...defaultConfigArgs, ...defaultIgnoreArgs, ...args, defaultFilesArgs ],
Copy link
Member

Choose a reason for hiding this comment

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

Guessing this is meant to be spread ...defaultFileArgs

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

aduth
aduth reviewed Jun 10, 2019
View reviewed changes
packages/scripts/README.md
@@ -25,10 +25,10 @@ _Example:_
"scripts": {
"build": "wp-scripts build",
"check-engines": "wp-scripts check-engines",
"check-licenses": "wp-scripts check-licenses --production",
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It validates all dependencies installed in the project, not only those used for production. We check regular and dev dependencies in Gutenberg, so I thought we should recommend both for other projects as well.

@aduth aduth mentioned this pull request Jun 10, 2019
Merged
@scruffian scruffian mentioned this pull request Apr 2, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@ntwb ntwb ntwb approved these changes

@swissspidy swissspidy Awaiting requested review from swissspidy

@oandregal oandregal Awaiting requested review from oandregal

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@nerrad nerrad Awaiting requested review from nerrad

@youknowriad youknowriad Awaiting requested review from youknowriad

@iandunn iandunn Awaiting requested review from iandunn

Assignees

@gziolo gziolo

Labels
[Package] Scripts /packages/scripts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 5.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gziolo @aduth @ntwb