feat(core/linter-rules): added linter for checking charset is utf-8 #2075

CodHeK · 2019-02-06T06:31:20Z

Closes #873

saschanaz

Also please remove builds/respec-w3c-common.js.map, as we only generate them when versioning.

src/w3c/defaults.js

CodHeK · 2019-02-06T06:57:56Z

@saschanaz updated the src/core/default.js file

saschanaz · 2019-02-06T07:43:27Z

Those failing tests don't know about the new linter, maybe we can add one for each of them.

tests/spec/w3c/defaults-spec.js

CodHeK · 2019-02-06T08:03:55Z

@saschanaz I don't get why travis-ci isin't passing ... could you please help me out with this .

tests/spec/w3c/defaults-spec.js

Co-Authored-By: CodHeK <[email protected]>

saschanaz · 2019-02-06T10:24:25Z

Run npm run lint -- --fix and those errors will disappear.

src/core/linter-rules/check-charset.js

tests/spec/core/linter-rules/check-charset-spec.js

CodHeK · 2019-02-06T13:06:49Z

@saschanaz made the changes you asked!

saschanaz · 2019-02-06T13:39:25Z

src/core/linter-rules/check-charset.js

+  const metas = doc.querySelectorAll("meta[charset]");
+  const val = [];
+  for (let i = 0; i < metas.length; i++) {
+    val.push(metas[i].outerHTML);


Maybe we can push meta.getAttribute("charset") instead? Then we can skip charset= part in getCharset function.

Also we prefer for-of loop: for (const meta of metas)

Question: Why would there be multiple meta[charset]?

@sidvishnoi No reason, it would always be by accident.

@sidvishnoi just considering a hypothetical case, cases wherein if someone just copied html from some other source, yet very rare

Author error maybe?

src/core/linter-rules/check-charset.js

CodHeK · 2019-02-06T15:34:36Z

Oh, okay so should I change the variable name ?

CodHeK · 2019-02-06T15:36:42Z

Okay, so I’ll get rid of the regex and use getAttribute and check if utf-8 exists in the val array ?

saschanaz · 2019-02-06T15:43:08Z

Okay, so I’ll get rid of the regex and use getAttribute and check if utf-8 exists in the val array ?

That would work.

CodHeK · 2019-02-06T16:17:27Z

@saschanaz what exactly should the linter return ( the object returned when utf8 is not present ) ?

src/core/linter-rules/check-charset.js

marcoscaceres

Couple of little things.

src/core/linter-rules/check-charset.js

marcoscaceres · 2019-02-06T16:44:06Z

src/core/defaults.js

@@ -24,6 +26,7 @@ export const coreDefaults = {
    "check-punctuation": false,
    "local-refs-exist": true,
    "check-internal-slots": false,
+    "check-charset": true,


Initially set this to false, so we can “beta test” it.

marcoscaceres · 2019-02-06T16:46:14Z

tests/spec/w3c/defaults-spec.js

@@ -15,6 +15,7 @@ describe("W3C — Defaults", () => {
      "local-refs-exist": true,
      "check-punctuation": false,
      "check-internal-slots": false,
+      "check-charset": true,


As above. Set it to flase by default. We will enable it after we test it out a bit.

marcoscaceres · 2019-02-06T16:56:03Z

src/core/linter-rules/check-charset.js

+  const utfExists = val.includes("utf-8");
+
+  //only a single meta[charset] and is set to utf-8, correct case
+  if (utfExists === true && metas.length === 1) {


Suggested change

if (utfExists === true && metas.length === 1) {

if (utfExists && metas.length === 1) {

marcoscaceres

Be sure to revert changes to the .map file too!

Starting to look pretty good 👍

CodHeK · 2019-02-06T17:25:34Z

Be sure to revert changes to the .map file too!

Starting to look pretty good 👍

@marcoscaceres .map file ?

CodHeK · 2019-02-06T17:32:52Z

tests/spec/w3c/defaults-spec.js

@@ -51,6 +52,7 @@ describe("W3C — Defaults", () => {
      "check-punctuation": false,
      "fake-linter-rule": "foo",
      "check-internal-slots": true,
+      "check-charset": true,


@marcoscaceres should this be set to true ? the karma test failed for this case ...

This one should be false, to match core/defaults.

@marcoscaceres done, sent a PR!

saschanaz

Also please do git checkout develop builds/*, this will restore builds/respec-w3c-common.js.map as the original state.

saschanaz · 2019-02-07T02:07:28Z

src/core/linter-rules/check-charset.js

+const meta = {
+  en: {
+    description: `Document must only contain one \`<meta>\` tag with charset set to 'utf-8'`,
+    howToFix: `Add this line in your document \`<head>\` section -  \`<meta charset="utf-8" />\` or set charset to "utf-8" if not set already.`,


Suggested change

howToFix: `Add this line in your document \`<head>\` section - \`<meta charset="utf-8" />\` or set charset to "utf-8" if not set already.`,

howToFix: `Add this line in your document \`<head>\` section - \`<meta charset="utf-8" />\` or set charset to "utf-8" if not set already.`,

saschanaz · 2019-02-07T02:07:45Z

src/core/linter-rules/check-charset.js

+ * Runs linter rule.
+ *
+ * @param {Object} config The ReSpec config.
+ * @param  {Document} doc The document to be checked.


Suggested change

* @param {Document} doc The document to be checked.

* @param {Document} doc The document to be checked.

saschanaz · 2019-02-07T02:08:00Z

src/core/linter-rules/check-charset.js

+/**
+ * Runs linter rule.
+ *
+ * @param {Object} config The ReSpec config.


Suggested change

* @param {Object} config The ReSpec config.

* @param {Object} conf The ReSpec config.

tests/spec/core/linter-rules/check-charset-spec.js

marcoscaceres

Looks like the only thing left to do is remove the changes to builds/respec-w3c-common.js.map

src/core/linter-rules/check-charset.js

marcoscaceres

LGTM1. @saschanaz ?

marcoscaceres · 2019-02-07T07:01:19Z

@CodHeK, I'm still seeing a mode change on builds/respec-w3c-common.js.map? Make sure you revert all changes to that file. Hopefully this will fix it:

git pull
git checkout develop builds/respec-w3c-common.js.map
git commit -b "revert changes" builds/respec-w3c-common.js.map

Waiting on builds/respec-w3c-common.js.map fix

marcoscaceres

Ok, looking good.

CodHeK mentioned this pull request Feb 6, 2019

Lint for charset not being UTF-8 #873

Closed

saschanaz requested changes Feb 6, 2019

View reviewed changes

src/w3c/defaults.js Outdated Show resolved Hide resolved

CodHeK added 3 commits February 6, 2019 12:21

added linter for checking charset is utf-8

08ed16c

deleted unecessary build

60c1a33

changed to core/default.js

0d41dc2

CodHeK force-pushed the feat_#873 branch from 9ee308a to 0d41dc2 Compare February 6, 2019 06:55

CodHeK added 3 commits February 6, 2019 12:30

resolved import error

555a9a3

resolved travis error

8866e07

resolved faling tests

e2d6f91

CodHeK changed the title ~~added linter for checking charset is utf-8~~ feat(core/linter-rules): added linter for checking charset is utf-8 Feb 6, 2019

added charset linter to w3c/defaults-spec.js

4bdbc27

saschanaz reviewed Feb 6, 2019

View reviewed changes

tests/spec/w3c/defaults-spec.js Outdated Show resolved Hide resolved

defaults-spec corrected

80b57cc

saschanaz requested changes Feb 6, 2019

View reviewed changes

tests/spec/w3c/defaults-spec.js Outdated Show resolved Hide resolved

Update tests/spec/w3c/defaults-spec.js

fefdfb8

Co-Authored-By: CodHeK <[email protected]>

Merge branch 'develop' into feat_#873

4a85dfa

saschanaz requested changes Feb 6, 2019

View reviewed changes

made changed

d30e53c

saschanaz reviewed Feb 6, 2019

View reviewed changes

src/core/linter-rules/check-charset.js Outdated Show resolved Hide resolved

updated linter

a858973

saschanaz requested changes Feb 6, 2019

View reviewed changes

src/core/linter-rules/check-charset.js Outdated Show resolved Hide resolved

src/core/linter-rules/check-charset.js Outdated Show resolved Hide resolved

changed linter return stmt

39486c0

marcoscaceres requested changes Feb 6, 2019

View reviewed changes

CodHeK added 2 commits February 6, 2019 22:18

fixed lint

47714d2

update defaults.js

c1b5e76

marcoscaceres reviewed Feb 6, 2019

View reviewed changes

CodHeK commented Feb 6, 2019

View reviewed changes

CodHeK added 3 commits February 6, 2019 23:11

update defaults-spec.js

d8d848c

fixed tests

5a336ca

fixed charset-test

84c9d57

saschanaz requested changes Feb 7, 2019

View reviewed changes

saschanaz reviewed Feb 7, 2019

View reviewed changes

tests/spec/core/linter-rules/check-charset-spec.js Outdated Show resolved Hide resolved

fixed spaces

b65e888

marcoscaceres reviewed Feb 7, 2019

View reviewed changes

tests/spec/core/linter-rules/check-charset-spec.js Outdated Show resolved Hide resolved

toEqual -> toBe

b8fb861

marcoscaceres reviewed Feb 7, 2019

View reviewed changes

src/core/linter-rules/check-charset.js Outdated Show resolved Hide resolved

added build file

1248e99

marcoscaceres previously approved these changes Feb 7, 2019

View reviewed changes

Merge branch 'develop' into feat_#873

2b5b660

CodHeK and others added 2 commits February 7, 2019 12:36

revert changed

1858b57

Merge branch 'develop' into feat_#873

396f828

marcoscaceres approved these changes Feb 7, 2019

View reviewed changes

fix another xhtml reference

f1ab861

saschanaz approved these changes Feb 7, 2019

View reviewed changes

marcoscaceres merged commit bd99374 into speced:develop Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core/linter-rules): added linter for checking charset is utf-8 #2075

feat(core/linter-rules): added linter for checking charset is utf-8 #2075

CodHeK commented Feb 6, 2019 •

edited by marcoscaceres

Loading

saschanaz left a comment

CodHeK commented Feb 6, 2019

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019 •

edited

Loading

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019

saschanaz Feb 6, 2019 •

edited

Loading

saschanaz Feb 6, 2019

sidvishnoi Feb 6, 2019

saschanaz Feb 6, 2019

CodHeK Feb 6, 2019

marcoscaceres Feb 6, 2019

CodHeK commented Feb 6, 2019 via email •

edited by saschanaz

Loading

CodHeK commented Feb 6, 2019 via email •

edited by saschanaz

Loading

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019

marcoscaceres left a comment

marcoscaceres Feb 6, 2019

marcoscaceres Feb 6, 2019

marcoscaceres Feb 6, 2019

marcoscaceres left a comment

CodHeK commented Feb 6, 2019

CodHeK Feb 6, 2019

marcoscaceres Feb 6, 2019

CodHeK Feb 6, 2019

saschanaz left a comment

saschanaz Feb 7, 2019

saschanaz Feb 7, 2019

saschanaz Feb 7, 2019

marcoscaceres left a comment

marcoscaceres left a comment

marcoscaceres commented Feb 7, 2019

marcoscaceres left a comment

	if (utfExists === true && metas.length === 1) {
	if (utfExists && metas.length === 1) {

	howToFix: `Add this line in your document \`<head>\` section - \`<meta charset="utf-8" />\` or set charset to "utf-8" if not set already.`,
	howToFix: `Add this line in your document \`<head>\` section - \`<meta charset="utf-8" />\` or set charset to "utf-8" if not set already.`,

	* @param {Document} doc The document to be checked.
	* @param {Document} doc The document to be checked.

	* @param {Object} config The ReSpec config.
	* @param {Object} conf The ReSpec config.

feat(core/linter-rules): added linter for checking charset is utf-8 #2075

feat(core/linter-rules): added linter for checking charset is utf-8 #2075

Conversation

CodHeK commented Feb 6, 2019 • edited by marcoscaceres Loading

saschanaz left a comment

Choose a reason for hiding this comment

CodHeK commented Feb 6, 2019

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019 • edited Loading

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019

saschanaz Feb 6, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodHeK commented Feb 6, 2019 via email • edited by saschanaz Loading

CodHeK commented Feb 6, 2019 via email • edited by saschanaz Loading

saschanaz commented Feb 6, 2019

CodHeK commented Feb 6, 2019

marcoscaceres 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

marcoscaceres left a comment

Choose a reason for hiding this comment

CodHeK commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz 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

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres commented Feb 7, 2019

marcoscaceres left a comment

Choose a reason for hiding this comment

CodHeK commented Feb 6, 2019 •

edited by marcoscaceres

Loading

CodHeK commented Feb 6, 2019 •

edited

Loading

saschanaz Feb 6, 2019 •

edited

Loading

CodHeK commented Feb 6, 2019 via email •

edited by saschanaz

Loading

CodHeK commented Feb 6, 2019 via email •

edited by saschanaz

Loading