Generally improved validation and added l10n validation #196

pkuczynski · 2018-04-06T13:02:19Z

add validation for l10n/css.json
verify unused keys in l10n/css.json - found a couple of unused keys and deleted them - was I right about it?
move all definitions into common definitions.js
generally improve the schema validation in few places

It would be possible to implement similar technique done for l10n also to definitions#enums, so there would be no need to manually define those enums, but instead validate if values exist in l10n and vice versa. What do you think about this idea?

frenic · 2018-04-06T13:16:03Z

Maybe sort order should be linted as well?

pkuczynski · 2018-04-06T13:20:00Z

ajv does not offer such validation, as JSON does not require props order. I can add it manually using javascript. Can you create a ticker for this?

pkuczynski · 2018-04-06T21:28:41Z

@frenic can you merge this PR?

frenic · 2018-04-06T21:35:54Z

I wish! 😆 I don't have the privilege to do that.

wbamberg · 2018-04-06T21:40:10Z

I do, but this is a big (though impressive!) change, and I probably won't have time to review it today.

@frenic, you can definitely help me by reviewing it, if you have the time.

BTW @pkuczynski , @lahmatiy , I want to make a new release of this repo as soon as possible, are there any open PRs you would really like to have merged before I do so? (or conversely, any potentially risky ones that you would not like merged?)

frenic

JavaScript is looking good and works fine. A small comment but it's really not a big deal.

I don't have the confidence enough to review the data changes though.

frenic · 2018-04-06T22:30:19Z

test/lint/checkStyle.js

+    console.log('  Style – OK')
+  } else {
+    noErrors = false
+    console.log('  Style – Error on line ' + jsonDiff(actual, expected))


It would be nice maybe if this displayed all the diffs rather than just one line at the time.

It's ok, jsonDiff returns all the diffs.

I understood the problem you pointed out. Yep, the logic were changed, but in another place (see my comment above).

frenic · 2018-04-06T22:34:43Z

#189 would be nice to have

pkuczynski · 2018-04-06T23:21:41Z

This PR doesn’t really change data, except l10n where I removed unused strings. All the rest are improvements on the validation

pkuczynski · 2018-04-06T23:23:49Z

There is a good question if we should not remove the whole enums in the definition.json and implement it differently using the same technique as for serching for unused translations? That would be my choice...

frenic · 2018-04-07T06:48:14Z

This PR doesn’t really change data, except l10n where I removed unused strings.

I meant that I haven't worked with JSON data to this extent before. So my knowledge about schemas is a bit limited.

frenic · 2018-04-07T07:01:01Z

There is a good question if we should not remove the whole enums in the definition.json and implement it differently using the same technique as for serching for unused translations? That would be my choice...

That sounds like a major change? @wbamberg In that case maybe it should be in another PR so it can be discussed separately?

pkuczynski · 2018-04-09T07:40:39Z

@wbamberg

Normalize media field values in css/properties.json and css/at-rules.json #193 and Add gap, row-gap and update column-gap CSS properties #189 are good candidates to merge before the release I think
adding matches pseudo-class #186 could be also merged after removing tokens mentioned by @lahmatiy. I can work on that if you want?
adding :host/:host()/:host-context() #171 could be merged once adding matches pseudo-class #186 is merged
attempting to add ::slotted pseudo-element and necessary syntaxes #169 not sure, maybe @teoli2003 could say something about it
Update initial values for prefix and suffix #162 could be fixed very quickly and worth merging

wbamberg · 2018-04-09T21:04:01Z

Please note that #189 needs a fix in l10n/css.json before I can merge it. I think the other issue there (removing grid-row-gap and grid-column-gap) doesn't have to block merging though.

pkuczynski · 2018-04-09T21:22:22Z

@wbamberg #189 fixed

# Conflicts: # css/properties.schema.json

pkuczynski · 2018-04-10T22:24:00Z

@wbamberg conflicts solved. Ready to merge!

lahmatiy

Great work!
I left a few notes.

lahmatiy · 2018-04-12T17:21:58Z

css/definitions.json

+      "$ref": "properties#/names"
+    }
+  },
+  "enums": {


Why we need to split types for paths enums/names/* and enums/text/*? I believe it can be a plain list of types, i.e. groups instead of enums/names/groups and initial instead of enums/text/initial

The reason why I split them, was because I am cross-checking enums/text with l10n/css. So if the key is missing in l10n or l10n has keys which does not exists in this list, the build will fail. This way I found some unused l10n strings. I felt like it is a good idea to keep things tidy.

I was also considering on dumping the whole enum/text and simply pull the values in js directly from individual props in *.json. This would limit the overhead of adding the new values into definitions.js. What do you think?

lahmatiy · 2018-04-12T17:35:50Z

test/lint.js

-var ajv = new Ajv({ $data: true, allErrors: true });
-var dictPaths = ['api', 'css', 'l10n'];
-var hasErrors = false;
+var fs = require('fs')


It's not fair, you have changed a code style 😢

Apologies. That was my editor default settings. Do you want me to bring back the semicolons?

It's not just a semicolons. Previously maintainers ask me to change a code style you found before your changes.
Also there is a big delta with a small effective changes that hard to find. I suggest to left changes in the script only about the main goal and move further refactoring to a separate PR.

lahmatiy · 2018-04-12T17:38:20Z

test/lint/checkStyle.js

+    console.log('  Style – OK')
+  } else {
+    noErrors = false
+    console.log('  Style – Error on line ' + jsonDiff(actual, expected))


It's ok, jsonDiff returns all the diffs.

lahmatiy · 2018-04-12T17:41:44Z

css/at-rules.json

@@ -54,7 +54,7 @@
      "prefix": {
        "syntax": "<symbol>",
        "media": "all",
-        "initial": "\"\"",


I believe it should be "\"\"" since it's a valid CSS value

I changed this so it will place nicely with l10n check. Can bring it back if you want?

lahmatiy · 2018-04-12T17:41:56Z

css/at-rules.json

@@ -81,7 +81,7 @@
      "suffix": {
        "syntax": "<symbol>",
        "media": "all",
-        "initial": "\". \"",


I believe it should be "\". \"" since it's a valid CSS value

lahmatiy · 2018-04-12T17:42:25Z

css/properties.json

@@ -188,7 +188,7 @@
    "groups": [
      "Microsoft Extensions"
    ],
-    "initial": "\"\"",


I believe it should be "\"\"" since it's a valid CSS value

lahmatiy · 2018-05-06T21:14:12Z

test/lint.js

-var ajv = new Ajv({ $data: true, allErrors: true });
-var dictPaths = ['api', 'css', 'l10n'];
-var hasErrors = false;
+var fs = require('fs')


It's not just a semicolons. Previously maintainers ask me to change a code style you found before your changes.
Also there is a big delta with a small effective changes that hard to find. I suggest to left changes in the script only about the main goal and move further refactoring to a separate PR.

lahmatiy · 2018-05-06T21:15:20Z

test/lint.js


-      console.log();
+      noErrors = noErrors && checkStyle(absFilename) && checkSchema(absFilename)


This change omits checking after the first fail. We want to see all the problems at once, not just first one.

lahmatiy · 2018-05-06T21:17:01Z

test/lint/checkStyle.js

+    console.log('  Style – OK')
+  } else {
+    noErrors = false
+    console.log('  Style – Error on line ' + jsonDiff(actual, expected))


I understood the problem you pointed out. Yep, the logic were changed, but in another place (see my comment above).

wbamberg · 2018-05-31T21:27:44Z

@pkuczynski , I'm sorry to have not reviewed this PR. It seems from your description that the PR does a few different things, would it be possible to split it up into separate PRs that each do one thing? That would greatly help me find the time to digest and review it.

pkuczynski · 2021-11-24T10:41:52Z

Shame it never got merged after 4y

pkuczynski added 2 commits April 6, 2018 14:44

Generally improved validation and added l10n validation

2ae2feb

Some minor fixes

496a0c6

pkuczynski mentioned this pull request Apr 6, 2018

Sort css.json alphabetically #188

Merged

frenic mentioned this pull request Apr 6, 2018

Lint sort order of data sheets #197

Open

Merge branch 'master' into more-validation

67cbcdb

frenic approved these changes Apr 6, 2018

View reviewed changes

wbamberg mentioned this pull request Apr 10, 2018

Regular NPM releases? #132

Closed

pkuczynski added 2 commits April 10, 2018 23:40

Merge branch 'master' into more-validation

cfb159f

# Conflicts: # css/properties.schema.json

Fix errors

8c994b1

lahmatiy reviewed Apr 12, 2018

View reviewed changes

lahmatiy suggested changes May 6, 2018

View reviewed changes

schalkneethling deleted the branch mdn:master November 24, 2021 09:49

schalkneethling closed this Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generally improved validation and added l10n validation #196

Generally improved validation and added l10n validation #196

pkuczynski commented Apr 6, 2018 •

edited

Loading

frenic commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

frenic commented Apr 6, 2018

wbamberg commented Apr 6, 2018 •

edited

Loading

frenic left a comment •

edited

Loading

frenic Apr 6, 2018

lahmatiy Apr 12, 2018

lahmatiy May 6, 2018

frenic commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

frenic commented Apr 7, 2018

frenic commented Apr 7, 2018

pkuczynski commented Apr 9, 2018 •

edited

Loading

wbamberg commented Apr 9, 2018

pkuczynski commented Apr 9, 2018

pkuczynski commented Apr 10, 2018

lahmatiy left a comment

lahmatiy Apr 12, 2018

pkuczynski Apr 13, 2018

lahmatiy Apr 12, 2018

pkuczynski Apr 13, 2018

lahmatiy May 6, 2018

lahmatiy Apr 12, 2018

lahmatiy Apr 12, 2018

pkuczynski Apr 13, 2018

lahmatiy Apr 12, 2018

lahmatiy Apr 12, 2018

lahmatiy May 6, 2018

lahmatiy May 6, 2018 •

edited

Loading

lahmatiy May 6, 2018

wbamberg commented May 31, 2018

pkuczynski commented Nov 24, 2021


		console.log();
		noErrors = noErrors && checkStyle(absFilename) && checkSchema(absFilename)

Generally improved validation and added l10n validation #196

Generally improved validation and added l10n validation #196

Conversation

pkuczynski commented Apr 6, 2018 • edited Loading

frenic commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

frenic commented Apr 6, 2018

wbamberg commented Apr 6, 2018 • edited Loading

frenic left a comment • 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

frenic commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

pkuczynski commented Apr 6, 2018

frenic commented Apr 7, 2018

frenic commented Apr 7, 2018

pkuczynski commented Apr 9, 2018 • edited Loading

wbamberg commented Apr 9, 2018

pkuczynski commented Apr 9, 2018

pkuczynski commented Apr 10, 2018

lahmatiy left a 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

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

lahmatiy May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbamberg commented May 31, 2018

pkuczynski commented Nov 24, 2021

pkuczynski commented Apr 6, 2018 •

edited

Loading

wbamberg commented Apr 6, 2018 •

edited

Loading

frenic left a comment •

edited

Loading

pkuczynski commented Apr 9, 2018 •

edited

Loading

lahmatiy May 6, 2018 •

edited

Loading