Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

Scripts: Add support for lint-pkg-json script #128

Merged
merged 2 commits into from
May 23, 2018
Merged

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 22, 2018

Follow-up for #119.

This PR adds a new script to @wordpress/scripts. It follows what we have already for npm test and introduces wp-scripts lint-pkg-json which helps enforce standards for your package.json file.

This change is required to make it available in Gutenberg, to make sure that packages published to npm follow the same setup as we have in this repository.

Testing

npm run lint:pkg-json - it should work as before. To test it try to change any package.json file in the packages folder.

@codecov
Copy link

codecov bot commented May 22, 2018

Codecov Report

Merging #128 into master will decrease coverage by 0.53%.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #128      +/-   ##
========================================
- Coverage   67.53%    67%   -0.54%     
========================================
  Files          57     58       +1     
  Lines         690    697       +7     
  Branches      144    145       +1     
========================================
+ Hits          466    467       +1     
- Misses        182    187       +5     
- Partials       42     43       +1
Impacted Files Coverage Δ
packages/scripts/scripts/lint-pkg-json.js 0% <0%> (ø)
packages/scripts/utils/package.js 100% <100%> (ø) ⬆️
packages/scripts/utils/index.js 97.29% <50%> (-2.71%) ⬇️

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 4beeb1a...670fc4b. Read the comment docs.

### `wp-scripts lint-pkg-json`

Helps enforce standards for your package.json file. It uses [npm-package-json-lint](https://www.npmjs.com/package/npm-package-json-lint) with the set of default rules provided. You can override them with your own rules as described in [npm-package-json-lint wiki](https://github.com/tclindner/npm-package-json-lint/wiki).

Copy link
Member

Choose a reason for hiding this comment

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

Let's include an example here:

  "scripts": {
    "lint:pkg-json": "wp-scripts lint-pkg-json ."
  },

Without the . at the end of the command this error is displayed: No lint targets provided

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good call. We can make sure it is provided as default instead. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

By default is a good idea if it can be overridden using packages/ for example with Lerna repos.

Regardless of that though we should include some basic docs here in the readme

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let's start with your example and iterate :)

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.

The docs request isn't a blocker for this to merge.

We should also look add more tests in the future

@gziolo
Copy link
Member Author

gziolo commented May 22, 2018

We should also look add more tests in the future

That's always the hardest part with CLI scripts - more mocking than actual testing :D

@@ -1,3 +1,7 @@
## 1.2.0 (2018-05-23)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we will wait for license checker before we release 1.2.0.

Copy link
Member

Choose a reason for hiding this comment

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

I think we will wait for license checker before we release 1.2.0.

So will this or won't this be published on 2018-05-23 ? I think we should either:

  • Not compile CHANGELOG.md edits until immediately before a published release.
    • This is what I thought had been settled upon in a previous core-js meeting (reference)
  • Not merge with an explicit date for a version until the date is known (either omit entirely, or a placeholder value like "Unreleased" or "TBD").

Regardless, we should codify this in CONTRIBUTING.md workflow documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Unreleased seems like a good choice in this case. I can publish new version just to make sure it doesn't lie 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, it is still unpublished, we should update docs and follow the flow @aduth described 👍

@gziolo gziolo merged commit 04093f5 into master May 23, 2018
@gziolo gziolo deleted the add/lint-pkg-json branch May 23, 2018 08:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants