Add `@wordpress/npm-package-json-lint-config` package #119

ntwb · 2018-05-06T09:12:11Z

This PR seeks to add a new package @wordpress/npm-package-json-lint-config

It implements a "shared config" for the package.json npm-package-json-lint linter

It is based on the upcoming currently unreleased v3 release of npm-package-json-lint

This was briefly discussed in last weeks #core-js chat

The benefit of being able to lint multiple package.json files allows us to ensure specific fields are included in our package.json files, for example, version, name, description, and publishConfig for Lerna which caught us out recently. npm-package-json-lint-config also triggers warnings and errors when fields use incorrect types, e.g. for when an object is expected yet an array is used for a package.json value. The configuration also forces an opinionated sort order of the package.json fields.

The shared configuration is relatively generic which will allow it to be used to validate WordPress projects package.json files, for example, core, bbPress, BuddyPress, Dashicons, grunt-patch-wordpress, and Gutenberg. For this repo the base configuration of @wordpress/npm-package-json-lint-config is extended by a npmPackageJsonLintConfig configuration option in the root package.json here.

This PR doesn't remove the manual check for publishConfig that was added in #115 as npm-package-json-lint currently only allows for checking if publishConfig exists, I've opened an issue here requesting that a valid-values-publishConfig rule be added so that we can enforce that the value of publishConfig is "access": "public"

…e-json-lint`

codecov · 2018-05-06T09:15:04Z

Codecov Report

Merging #119 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   67.44%   67.53%   +0.09%     
==========================================
  Files          56       57       +1     
  Lines         688      690       +2     
  Branches      144      144              
==========================================
+ Hits          464      466       +2     
  Misses        182      182              
  Partials       42       42

Impacted Files	Coverage Δ
packages/npm-package-json-lint-config/index.js	`100% <100%> (ø)`

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 8a2dea2...63f6b74. Read the comment docs.

gziolo · 2018-05-07T07:04:25Z

scripts/create-symlinks.js

@@ -12,6 +12,7 @@ mkdirp( 'node_modules/@wordpress', () => {
 		[ 'packages/browserslist-config', 'node_modules/@wordpress/browserslist-config' ],
 		[ 'packages/babel-preset-default', 'node_modules/@wordpress/babel-preset-default' ],
 		[ 'packages/jest-preset-default', 'node_modules/@wordpress/jest-preset-default' ],
+		[ 'packages/npm-package-json-lint-config', 'node_modules/@wordpress/npm-package-json-lint-config' ],


I was wondering if we can create this list using glob?

Possibly, will follow up in another PR

aduth · 2018-05-08T12:58:50Z

packages/npm-package-json-lint-config/package.json

@@ -0,0 +1,30 @@
+{
+  "name": "@wordpress/npm-package-json-lint-config",


Any way we can enforce tabs for indentation? 😉

I've requested a new rule to prevent my future self facepalming:

tclindner/npm-package-json-lint#81

aduth · 2018-05-08T12:58:55Z

packages/npm-package-json-lint-config/package.json

+    "type": "git",
+    "url": "git+https://github.com/WordPress/packages.git"
+  },
+	"bugs": {


Whitespace consistency.

Thanks, fixed in 963eae1

…ion for _packages_ repo

…agejsonlintrc.json`

gziolo · 2018-05-15T13:35:42Z

package.json

@@ -56,7 +56,7 @@
 		"check-engines": "check-node-version --package",
 		"create-symlinks": "node ./scripts/create-symlinks.js",
 		"lerna-bootstrap": "lerna bootstrap --hoist",
-		"npmpackagejsonlint": "npmPkgJsonLint ./packages -c .npmpackagejsonlintrc.json",
+		"npmpackagejsonlint": "npmPkgJsonLint ./packages",


Should it be named lint-packages?

What about lint:pj, not that it is immediately obvious what actually infers I saw it the other day alongside lint:js, lint:md etc?

"lint:js": "eslint .", "lint:css": "stylelint .", "lint:md": "remark-lint .", "lint:pj": "npmPkgJsonLint ./packages", "lint:scss": "stylelint --syntax=scss .",

Or lint:package maybe?, could be used with npm-run-all npm run all lint:*

lint:pkg-json should do the trick, and agreed on npm-run-all.

gziolo

Let's proceed with this one. LGTM 👍

I left 2 nitpicks to address before merging.

gziolo · 2018-05-17T09:21:41Z

packages/npm-package-json-lint-config/test/index.test.js

@@ -0,0 +1,14 @@
+'use strict';


I don't think we need to use use strict in JS file anymore, because we use es6 modules.

gziolo · 2018-05-17T09:22:07Z

packages/npm-package-json-lint-config/test/index.test.js

@@ -0,0 +1,14 @@
+'use strict';
+
+const config = require( '../' );


You can use import statement here.

gziolo · 2018-05-17T09:32:29Z

is-plain-obj is not added as a dependency, but it works because it is included by something else 😄

gziolo · 2018-05-17T09:35:48Z

I addressed my own comments, let's get it in to take advantage of linting :)

aduth · 2018-05-23T12:44:16Z

package.json

@@ -11,8 +11,10 @@
 		"check-node-version": "^3.1.1",
 		"codecov": "^2.3.1",
 		"glob": "^7.1.2",
+		"is-plain-obj": "^1.1.0",


Does this need to be defined as a devDependencies in the root if we're not using it in the root?

Package are local, tests are global, so we just need pick one approach for those dependencies. I have a feeling that other packages define them in their own package.json file. Lerna will hoist, them so we don't need it in here.

Okay. Seemed we should need it to be defined in either root or npm-package-json-lint-config/package.json, but not both.

Yes, one of them is enough.

aduth · 2018-05-23T12:47:12Z

packages/npm-package-json-lint-config/test/index.test.js

+
+describe( 'npm-package-json-lint config tests', () => {
+	it( 'should be an object', () => {
+		expect( isPlainObj( config ) ).toBeTruthy();


The toBeTruthy assertion should be discouraged, as it implies that we expect it could be a value other than explicitly true (a non-zero number, a non-empty string, etc). If we expect it should be explicitly true, we should assert as toBe( true ).

aduth · 2018-05-23T12:50:30Z

packages/npm-package-json-lint-config/test/index.test.js

+/**
+ * External dependencies
+ */
+import isPlainObj from 'is-plain-obj';


Minor: Not sure if introduction of dependency here is strictly necessary. What is it we're testing?

Would expect.toMatchObject( {} ) have been adequate, particularly if we don't care about subset of properties?

Alternatively, Lodash includes an _.isPlainObject as well, if we want to lean on Lodash as a canonical source of utility functions.

Thinking in terms of being explicit with where dependencies add value to avoid excessive proliferation.

ntwb added 3 commits May 6, 2018 14:58

Add @wordpress/npm-package-json-lint-config package

418454c

Standardize packages package.json format per `@wordpress/npm-packag…

a32b5e2

…e-json-lint`

Add unused npm-package-json-lint rules for reference

0f69270

ntwb added the New Package label May 6, 2018

gziolo reviewed May 7, 2018

View reviewed changes

aduth reviewed May 8, 2018

View reviewed changes

ntwb added 7 commits May 10, 2018 10:16

Update npm-package-json-lint to 3.0.0-alpha3

3d305a8

Add valid-values-publishConfig rule with {"access": "public"} opt…

c1fcbe2

…ion for _packages_ repo

use tabs for indentation

963eae1

Merge branch 'master' into add/npm-package-json-lint-config

878c20b

Update npm-package-json-lint to v3

75e27e4

Update npmPackageJsonLintConfig config formatting

ecfe8a6

Use latest 3.0.1 npm-package-json-lint release and remove `.npmpack…

449dee8

…agejsonlintrc.json`

gziolo reviewed May 15, 2018

View reviewed changes

ntwb added 2 commits May 16, 2018 00:22

Use lint:pkg-json as the npmPkgJsonLint script task name

438f41d

fix pretest test script command

e497ec6

gziolo approved these changes May 17, 2018

View reviewed changes

Npm package.json lint config: address my own comments

8b8e73e

Add is-plain-object as devDependency

63f6b74

gziolo merged commit 240be25 into master May 17, 2018

gziolo deleted the add/npm-package-json-lint-config branch May 17, 2018 09:39

gziolo mentioned this pull request May 22, 2018

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

Merged

aduth reviewed May 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `@wordpress/npm-package-json-lint-config` package #119

Add `@wordpress/npm-package-json-lint-config` package #119

ntwb commented May 6, 2018 •

edited

Loading

codecov bot commented May 6, 2018 •

edited

Loading

gziolo May 7, 2018

ntwb May 15, 2018

aduth May 8, 2018

ntwb May 10, 2018

aduth May 8, 2018

ntwb May 10, 2018

gziolo May 15, 2018 •

edited

Loading

ntwb May 15, 2018

ntwb May 15, 2018

gziolo May 15, 2018

gziolo left a comment

gziolo May 17, 2018

gziolo May 17, 2018

gziolo commented May 17, 2018

gziolo commented May 17, 2018

aduth May 23, 2018

gziolo May 23, 2018

aduth May 23, 2018

gziolo May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

		@@ -0,0 +1,30 @@
		{
		"name": "@wordpress/npm-package-json-lint-config",

		@@ -0,0 +1,14 @@
		'use strict';

		const config = require( '../' );

Add @wordpress/npm-package-json-lint-config package #119

Add @wordpress/npm-package-json-lint-config package #119

Conversation

ntwb commented May 6, 2018 • edited Loading

codecov bot commented May 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented May 17, 2018

gziolo commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `@wordpress/npm-package-json-lint-config` package #119

Add `@wordpress/npm-package-json-lint-config` package #119

ntwb commented May 6, 2018 •

edited

Loading

codecov bot commented May 6, 2018 •

edited

Loading

gziolo May 15, 2018 •

edited

Loading