Lint with tabs & enable strict mode #90

awwright · 2020-07-14T21:33:49Z

This adds an .eslintrc.js, .editorconfig, and normalizes the whitespace to 1-tab indents.

Btw, whitespace can be ignored from GitHub by adding ?w=1 to a diff URL: 2bbb7e2?w=1

This is mostly just a suggestion, it's easy enough to go with a different style if something else is more popular.

After this:

Fix remaining eslint errors with switch/break statements
Add eslint to CI tests
Enable additional eslint tests

brodybits · 2020-07-14T22:22:41Z

Thanks. I have mixed feelings about using tabs at this point. My biggest worry with tabs is that they do not look so nice on Unix editors, especially on Linux & Unix.

I am starting to see your point in #29 that tabs can help avoid issues like 7-space indentation and 2-space vs 4-space indentation. It is also nice to see not all lines of the code suddenly reindented with spaces. I would be interested in what the other contributors & maintainers have to say about this idea.

It is possible to configure eslint in a YML file, which may be a little easier for us to update over time. It would also be possible to configure eslint per subdirectory tree, which would make it possible to format lib and test in separate PRs. An idea might be to format the tests first, then continue fixing the assertions in #89 before cleaning up lib.

I am also thinking we could use Prettier at the same time to solve both indentation and code formatting at the same time. I think this would be better than the extra code churn that would come from doing this in multiple steps. I think it should be pretty straightforward to add eslint and Prettier together using eslint-plugin-prettier.

A final comment on lib is that the test coverage of lib/entities.js is pretty bad, as you can see by following the report from the mutation score badge. It might be nice to cover remaining entries in lib/entities.js before reformatting it.

awwright · 2020-07-14T23:30:36Z

Thanks, I appreciate the consideration. Also note:

GitHub reads .editorconfig, and will set tab widths to whatever is specified there.
On Linux and macOS you can run tabs -4 (or put it in your shell rc) which sets the tab stops to every 4 columns. (or whatever other number you like).

brodybits · 2020-07-15T00:02:42Z

That makes sense for me, and thanks for the tip to use ?w=1. I took the liberty to update the title, would like to give others some time for feedback before continuing with a possible merge.

Do you think my idea to use eslint-plugin-prettier to clean up the formatting in one shot makes sense?

awwright · 2020-07-15T01:17:24Z

@brodybits I don't have any experience with that, but doing more rather than less makes sense to me. I've usually found --fix to be sufficient for formatting, except for some mixed-spaces-tabs problems.

brodybits · 2020-08-13T22:01:11Z

I saw an excellent argument recently on Twitter: accessibility

I think I lost the first one I saw, think I found another one:

I was firmly in the camp of spaces only in source code (entered with the tab key, of course, and proper editor settings) until I read this about the accessibility benefits of using tab characters for indentation:https://t.co/TIASFjXcL8
— Tommy Williams (@twwilliams) January 24, 2020

https://www.reddit.com/r/javascript/comments/c8drjo/nobody_talks_about_the_real_reason_to_use_tabs/

karfau

I like the idea of adding a minimal linting setup, also regarding doing more of it if feasible, but I'm not so sure about applying "use strict"; before having more tests.

karfau · 2020-08-17T03:09:52Z

lib/dom.js

@@ -1,3 +1,5 @@
+"use strict";


I don't have a clear understanding of the impact, might this be a breaking change?
According to MDN "Strict mode changes both syntax and runtime behavior.".
So I would rather wait for more tests to be available before making such a change.

It's possible to statically determine all of the new errors strict mode might throw. Iirc, eslint raises errors for all of these cases by default.

karfau · 2020-08-17T03:13:15Z

lib/dom.js

+function getTextContent(node){
+	switch(node.nodeType){
+	case ELEMENT_NODE:
+	case DOCUMENT_FRAGMENT_NODE:
+		var buf = [];
+		node = node.firstChild;
+		while(node){
+			if(node.nodeType!==7 && node.nodeType !==8){
+				buf.push(getTextContent(node));
+			}
+			node = node.nextSibling;
+		}
+		return buf.join('');
+	default:
+		return node.nodeValue;
+	}
+}


What is the reason for moving this function here? At first glance is seems to be unrelated to the linting changes. If so you could extract it into a separate PR.

This function was previously inside an "if" statement, which is ambiguous behavior because of function hoisting; this is prohibited in strict mode.

karfau · 2020-08-17T12:25:47Z

Maybe we can already land the .editorconfig in an extra PR? So that all new contributions don't need to change when this PR lands?

brodybits · 2020-08-17T13:31:39Z

Maybe we can already land the .editorconfig in an extra PR? So that all new contributions don't need to change when this PR lands?

+1 I will likely do that soon

--

Sent from my mobile

.editorconfig

as originally proposed in PR #90 Co-authored-by: Austin Wright <aaa@bzfx.net> Co-authored-by: Chris Brody <chris.brody+brodybits@gmail.com>

brodybits

Thanks again to @awwright for your work on this.

Unfortunately I think we need to make these updates in multiple focused PRs, which I have already started working on.

More details are coming in a separate comment, since GitHub did not seem to do proper cross-referencing from review comments.

brodybits · 2020-08-18T02:33:16Z

I have started making these changes in the following PRs:

Add .editorconfig file #104 - add .editorconfig now merged ✅
eslint with tabs; remove trailing spaces #102 - eslint with tabs & remove trailing whitespace
eslint with Prettier formatting [with semicolons] #110 - eslint with Prettier formatting

As I said in PR #110, I would now favor that we do the formatting with Prettier all at once.

I think the changes to enable strict mode are also needed and should be done after we have finished with the eslint and/or Prettier formatting.

brodybits changed the title ~~Lint~~ Lint with tabs Jul 14, 2020

brodybits mentioned this pull request Jul 17, 2020

remove ES6 syntax from getElementsByClassName #91

Merged

karfau reviewed Aug 17, 2020

View reviewed changes

brodybits mentioned this pull request Aug 17, 2020

test: Restructure & improve npm test scripts #87

Merged

brodybits reviewed Aug 17, 2020

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

This was referenced Aug 17, 2020

eslint with tabs; remove trailing spaces #102

Closed

Add .editorconfig file #104

Merged

brodybits added a commit that referenced this pull request Aug 17, 2020

Add .editorconfig file (#104)

3ceec5e

as originally proposed in PR #90 Co-authored-by: Austin Wright <aaa@bzfx.net> Co-authored-by: Chris Brody <chris.brody+brodybits@gmail.com>

brodybits changed the title ~~Lint with tabs~~ Lint with tabs & enable strict mode Aug 17, 2020

brodybits added the bug Something isn't working label Aug 17, 2020

brodybits requested changes Aug 18, 2020

View reviewed changes

awwright added 2 commits September 16, 2020 14:02

Add .eslintrc.js

292fd34

Appease linter: normalize whitespace

86cdabb

awwright force-pushed the lint branch from 2bbb7e2 to 86cdabb Compare September 16, 2020 21:10

awwright closed this Sep 17, 2020

brodybits mentioned this pull request Sep 18, 2020

apply Prettier formatting --no-semi in test only #123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint with tabs & enable strict mode #90

Lint with tabs & enable strict mode #90

awwright commented Jul 14, 2020 •

edited

brodybits commented Jul 14, 2020

awwright commented Jul 14, 2020

brodybits commented Jul 15, 2020

awwright commented Jul 15, 2020

brodybits commented Aug 13, 2020

karfau left a comment

karfau Aug 17, 2020

awwright Aug 17, 2020

karfau Aug 17, 2020

awwright Aug 17, 2020 •

edited

karfau commented Aug 17, 2020

brodybits commented Aug 17, 2020 via email

brodybits left a comment

brodybits commented Aug 18, 2020

Lint with tabs & enable strict mode #90

Lint with tabs & enable strict mode #90

Conversation

awwright commented Jul 14, 2020 • edited

brodybits commented Jul 14, 2020

awwright commented Jul 14, 2020

brodybits commented Jul 15, 2020

awwright commented Jul 15, 2020

brodybits commented Aug 13, 2020

karfau left a comment

Choose a reason for hiding this comment

karfau Aug 17, 2020

Choose a reason for hiding this comment

awwright Aug 17, 2020

Choose a reason for hiding this comment

karfau Aug 17, 2020

Choose a reason for hiding this comment

awwright Aug 17, 2020 • edited

Choose a reason for hiding this comment

karfau commented Aug 17, 2020

brodybits commented Aug 17, 2020 via email

brodybits left a comment

Choose a reason for hiding this comment

brodybits commented Aug 18, 2020

awwright commented Jul 14, 2020 •

edited

awwright Aug 17, 2020 •

edited