Allow specs to be represented by a simple string #3

tidoust · 2020-01-21T11:37:48Z

In most cases, a URL should be all we need in that list. Getting rid of the wrapping { "url": ... } struct in these cases allows to keep the list more human readable.

This update allows entries to be just a string. Linting code updated to prefer the string format when there is only a URL (which is going to be the case as long as we don't create additional properties). Specifications remain sorted by alphabetic order of their URL.

The index.js file continues to export an array of objects with a url property to ease ingestion by external tools.

See discussion in:
#2 (review)

(Note that if we actually discover later on that a URL is not enough in most cases, we may want to undo this change...)

In most cases, a URL should be all we need in that list. Getting rid of the wrapping `{ "url": ... }` struct in these cases allows to keep the list more human readable. This update allows entries to be just a string. Linting code updated to prefer the string format when there is only a URL (which is going to be the case as long as we don't create additional properties). Specifications remain sorted by alphabetic order of their URL. The `index.js` file continues to export an array of objects with a `url` property to ease ingestion by external tools. See discussion in: w3c#2 (review) (Note that if we actually discover later on that a URL is **not** enough in most cases, we may want to undo this change...)

index.js

lint.js

foolip · 2020-01-21T12:42:36Z

lint.js

+
+  const sorted = specs
+    .map(spec => isString(spec) ? { url: spec } : spec)
+    .map(spec => Object.assign({}, spec, { url: new URL(spec.url).toString() }));


This allows for arbitrary additional properties, most likely in the form of typos if we add more properties. I'd prefer this code to normalize everything that is known and drop anything else. Since currently no additional properties are allowed, just require all entries to be a string.

I was assuming that we would in any case add some kind of JSON schema to validate the structure before we pass it to the linter. I'll add some code logic to that effect.

I started out using ajv myself, but ripped it out since the schema was so trivial. Now when either a string or object are allowed it's less trivial, so I'm happy with using it.

I'll have a few days off for the lunar new year, but will review this when I'm back next week!

I just added a JSON Schema and schema validation. The schema is a bit dumb for now since it allows an object with a URL, but then that's useless as long as we don't have additional properties, especially since the linting process will convert objects to URL strings. At least we'll have the right skeleton and logic in place for when we add properties.

Also added a method to slightly improve error messages returned by the JSON Schema validation to make them more human-friendly.

Make sure that the linting process accepts correct lists of specs, lints lists of specs that can be linted, and throws when something is wrong. Tests done with mocha. Run `npm test` to run the tests.

tidoust · 2020-01-24T21:52:46Z

Note I added schema validation and created tests for the linter.

.github/workflows/lint.yml

foolip · 2020-01-31T09:07:49Z

lint.js

-  let specs = JSON.parse(specsBuffer);
+// When an entry is invalid, the schema validator returns one error for each
+// "oneOf" option and one error on overall "oneOf" problem. This is confusing
+// for humans. the following function improves the error being returned.


Suggested change

// for humans. the following function improves the error being returned.

// for humans. The following function improves the error being returned.

foolip · 2020-01-31T09:08:16Z

lint.js

+// "oneOf" option and one error on overall "oneOf" problem. This is confusing
+// for humans. the following function improves the error being returned.
+const clarifyErrors = errors => {
+  if (!errors || (errors.length < 2)) {


Suggested change

if (!errors || (errors.length < 2)) {

if (!errors || errors.length < 2) {

foolip · 2020-01-31T09:09:09Z

lint.js

+  }
+
+  // Otherwise, if second error is a type error for second oneOf choice,
+  // it means the itme is actually a string that represents an invalid URL,


Suggested change

// it means the itme is actually a string that represents an invalid URL,

// it means the item is actually a string that represents an invalid URL,

foolip · 2020-01-31T09:09:24Z

lint.js

+
+  // Otherwise, if second error is a type error for second oneOf choice,
+  // it means the itme is actually a string that represents an invalid URL,
+  // which the first error should capture


Suggested change

// which the first error should capture

// which the first error should capture.

foolip · 2020-01-31T09:16:10Z

specs.json

-  {
-    "url": "https://compat.spec.whatwg.org/"
-  }
+  "http://compat.spec.whatwg.org/"


Should be http://compat.spec.whatwg.org/. I'll extend the lint to require this.

should be https://compat.spec.whatwg.org/ rather, right?

Indeed, I missed the s. I guess it shows it's a good idea to add HTTPS enforcement to the linter...

#3

#4) #3

tidoust requested review from dontcallmedom and foolip January 21, 2020 11:37

foolip requested changes Jan 21, 2020

View reviewed changes

tidoust added 3 commits January 21, 2020 17:47

Simplify string detection logic

6f77ab1

Add JSON Schema and schema validation to linting logic

82aeffd

Also added a method to slightly improve error messages returned by the JSON Schema validation to make them more human-friendly.

Add tests for linter

55acd74

Make sure that the linting process accepts correct lists of specs, lints lists of specs that can be linted, and throws when something is wrong. Tests done with mocha. Run `npm test` to run the tests.

foolip reviewed Jan 31, 2020

View reviewed changes

.github/workflows/lint.yml Show resolved Hide resolved

foolip approved these changes Jan 31, 2020

View reviewed changes

foolip merged commit dd12ab4 into w3c:master Jan 31, 2020

foolip added a commit that referenced this pull request Jan 31, 2020

Fix nits/typos from "Allow specs to be represented by a simple string"

31f6486

#3

foolip mentioned this pull request Jan 31, 2020

Fix nits/typos from "Allow specs to be represented by a simple string" #4

Merged

tidoust pushed a commit that referenced this pull request Jan 31, 2020

Fix nits/typos from "Allow specs to be represented by a simple string" (

8fed617

#4) #3

tidoust deleted the urlonly branch June 3, 2020 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specs to be represented by a simple string #3

Allow specs to be represented by a simple string #3

tidoust commented Jan 21, 2020

foolip Jan 21, 2020

tidoust Jan 21, 2020

foolip Jan 23, 2020

tidoust Jan 23, 2020

tidoust commented Jan 24, 2020

foolip Jan 31, 2020

foolip Jan 31, 2020

foolip Jan 31, 2020

foolip Jan 31, 2020

foolip Jan 31, 2020

dontcallmedom Jan 31, 2020

tidoust Jan 31, 2020

	// for humans. the following function improves the error being returned.
	// for humans. The following function improves the error being returned.

	if (!errors \|\| (errors.length < 2)) {
	if (!errors \|\| errors.length < 2) {

	// it means the itme is actually a string that represents an invalid URL,
	// it means the item is actually a string that represents an invalid URL,

	// which the first error should capture
	// which the first error should capture.

Allow specs to be represented by a simple string #3

Allow specs to be represented by a simple string #3

Conversation

tidoust commented Jan 21, 2020

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

tidoust commented Jan 24, 2020

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

Choose a reason for hiding this comment