Apply Editorconfig checking to the build

[nschonni]

Adds https://github.com/jedmao/eclint to the lint and fix targets to apply those settings from the EditorConfig file.
The tool is not language aware, so it doesn't really format the indenting like a full IDE, but does best guess to remove extra leading space. There were a few places where it looked like adding rounding the spaces up rather than down was better so I manually changed those.
I also added the setting for comment blocks which added to the number of JS files touched.

---

Preview | Diff

[mcking65]

@nschonni, looks like a great addition to the build process.

Given this touches 134 files, my primary concern with merging now is the potential headache for everyone actively working in branches right now. We have a bunch of active projects in flight at this time. They will all have to rebase or merge master into their work, and it looks like this could create a ton of conflicts. Or, will git be able to automatically resolve all them just because they are white space changes? Will it get them right?

I wonder if it would be better to wait until we have fewer people and projects that could be effected. Closer to December release time, the major projects will all be merged into master.

[nschonni]

Or, will git be able to automatically resolve all them just because they are white space changes? Will it get them right?

Not sure, I think when the same line is editted, then there will be conflicts.

I wonder if it would be better to wait until we have fewer people and projects that could be effected

That's fine with me. I split this into 3 commits, and the last one is the one that contains all the file changes. When it's ready that commit can be redone.

Alternately, this could be turned on for selected folders that don't have pending PRs and eventually changed to a global check

[mcking65]

@sh0ji, the stylelint work is now successfully merged into all 10 active branches. So, we are ready to go forward with work on this one.

[nschonni]

Bad rebase, hold on

[nschonni]

@sh0ji I'm running into an issue with the Husky setup

� eslint --fix found some errors. Please fix them and try committing again.

Cannot read config file: Z:\Workspaces\aria-practices\test\.eslintrc.json
Error: Cannot read property 'content-encoding' of undefined
TypeError: Cannot read config file: Z:\Workspaces\aria-practices\test\.eslintrc.json
Error: Cannot read property 'content-encoding' of undefined
  at module.exports.res (Z:\Workspaces\aria-practices
ode_modules\strip-json-comments\index.js:7:45)
  at loadJSONConfigFile (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config\config-file.js:114:27)
  at loadConfigFile (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config\config-file.js:225:26)
  at loadFromDisk (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config\config-file.js:495:18)
  at Object.load (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config\config-file.js:559:20)
  at Config.getLocalConfigHierarchy (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config.js:227:44)
  at Config.getConfigHierarchy (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config.js:179:43)
  at Config.getConfigVector (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config.js:286:21)
  at Config.getConfig (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\config.js:329:29)
  at processText (Z:\Workspaces\aria-practices
ode_modules\eslint\lib\cli-engine.js:163:33)
husky > pre-commit hook failed (add --no-verify to bypass)

[sh0ji]

Very weird, I'm seeing the same thing on your branch when I run npm run lint:es, but it's not happening on master. It seems to happen regardless of how you store the eslint config.

[nschonni]

It might be windows related since I rebased there last. I'll probably look at adding the encoding and line ending of lf to both the editorconfig and .gitattributes to see if that helps

[sh0ji]

On master, I just tried installing eclint, adding the scripts from b347935, updating the .editorconfig to match b5c1153, and npm run lint:es doesn't error. This makes me think the issue lies somewhere in 8d97b56.

[nschonni]

Redid this on my Mac and it seems happy again. I'll try pulling this down on my windows machine later to see if I get the same issue

[sh0ji]

Great, it's working for me as well.

Random thought: is there a risk in using eclint fix to modify files that eslint or stylelint have already fixed? I suspect at the very least, it means we'd have to maintain configs in more than one place.

[nschonni]

I don't think so, because ECLint is kind of simple. It will mostly just convert tabs and spaces to their equivalent and basic line endings. StyleLint and ESLint have more smarts, and will indent with context of function levels. I think the only time that they'd conflict is if the settings conflict (EX: EditorConfig says 3 spaces and ESLint say 4).

[mcking65]

Double checking to make sure that all the changes in c61c9aa are strictly white space, I did the following:

$ git diff -w --ignore-space-at-eol --ignore-blank-lines 6f79417..c61c9aa
diff --git a/examples/menubar/menubar-1/images/separator.paint b/examples/menubar/menubar-1/images/separator.paint
index 9e313a1..d6ccb88 100644
Binary files a/examples/menubar/menubar-1/images/separator.paint and b/examples/menubar/menubar-1/images/separator.paint differ
diff --git a/examples/slider/images/max-arrow.paint b/examples/slider/images/max-arrow.paint
index 232fa40..17a9070 100644
Binary files a/examples/slider/images/max-arrow.paint and b/examples/slider/images/max-arrow.paint differ

Why would these 2 binary files have been touched by this? I'm not sure whether this is a concern.

[nschonni]

I had ignored one of the paint files, but I'll added the file type instead since I guess those got added after my first pass. Might be better to remove them if they aren't needed, but that can be a separate thing

[jongund]

We should get rid of the paint files and at some point covert the arrows to SVG. But for now we should just delete the paint files.

[sh0ji]

The task force has some concerns about maintaining this file since there seem to be quite a few exceptions to the rules.

@mcking65, I suspect the ideal way to handle this would be to keep all of our dependencies and vendor files separate from the project source so that we could just ignore those files entirely (starting to feel like node_modules). As it is, our dependencies are in quite a few different places, which Nick enumerated in

aria-practices/.editorconfig

Line 33 in c61c9aa

[{/common/**,/examples/landmarks/js/visua11y.js,/examples/landmarks/css/bootstrap.css,/examples/landmarks/css/bootstrap-accessibility.css,/examples/menubar/menubar-1/images/separator.paint}]

.

@nschonni, do you have any suggestions for how we could make this more maintainable?

[nschonni]

Not unless you decide to go your path of pull them out of the repo. These files are already ignored by both ESLint and StyleLint

[mcking65]

Now that we are having editorconfig touch every file commited, I'm wondering why have the first section of .editorconfig that touches every file?

Normally, editorconfig is only used by an editor when a file is editted. And, typically, people do not edit binaries with editors that use ec. However, when running it this way, isn't it touching every single file that gets commited? That would then touch lots of files that ec would normally not touch.

If I'm understanding correctly, we could simplify just by telling ec to only work on specific types of files. Then, we need only ignore 3rd party files of those types.

[mcking65]

@jongund, if we are going to get rid of the paint files, we should do that separately, not part of this PR.

So, you say delete them ... does that mean they are never used? If so, I'd be happy to have a separate PR that only deletes the unused files.

[nschonni]

I can try pushing the values currently set under the [*] values to the language specific setting, but I'm unsure if the files would get touched, but use defaults. Alternately, I can change the glob from . to something that specifies the extensions only like .{js,html,css,md}

[mcking65]

Oops, @nschonni, now I've created conflicts by merging #993.

@nschonni commented:

I can try pushing the values currently set under the [*] values to the language specific setting,

That seems reasonable to me.

but I'm unsure if the files would get touched, but use defaults.

Are you thinking that if we do that, then EC would go ahead and touch every file anyway and use some set of defaults rather than we what we now have set for all files? I hope it doesn't work that way.

Alternately, I can change the glob from . to something that specifies the extensions only like .{js,html,css,md}

I'm not clear what you mean with this alternative. Where would this change be made? Do you mean change line 16 in .editorconfig to specify file types? That seems equivalent to the first option you suggested.

Basically, my concern is that it doesn't seem like we should let EC have any opinions about unknown file types. We know exactly what we want in js, css, html, and md. In any other type of file, the format really doesn't matter. If there is another type for which we have opinions, we should add it specifically rather than apply some default opinions indiscriminately. For instance, maybe we care about encoding and line endings in svg and txt files; if so, we could add those specifically. But, we should not have to explicitly exclude every binary file that lands in the repo.

[nschonni]

Rebased and added a .gitattributes to give a layer of end line cleanup at the source control layer

[mcking65]

@nschonni ... I assume there is a white space conflict somewhere in that block starting at line 5006 in aria-practices.html? I can't find the difference.

[nschonni]

Yeah, looks like a tab/space thing. I'll try rebasing again later today

[mcking65]

Instead of applying to all files, shouldn't we explicitly state which files we consider text? I get concerned about git thinking a file is text but it is binary. In the future, if someone adds a binary file, isn't aware of this, and if git makes a mistake, it could cause problems.

[nschonni]

You can, but this is the common file patter you'll see in other repos. If a new binary files get added, then i believe it will just try to treat it as diff'able. I think that would be same behaviour as today without this file though

[mcking65]

Unfortunately, I'm really struggling with this pr b/c Chrome and Firefox both seem to hang on me when I use most features on the files tab or even if I open a single commit. I don't know if it is realted to JAWS or not, but I don't think so because it is a script execution problem.

I am looking at the files locally, but that makes it harder to see what has changed.

[nschonni]

Is the Patch URL any better https://patch-diff.githubusercontent.com/raw/w3c/aria-practices/pull/856.patch

[nschonni]

@mcking65 rebased for the conflict

[mcking65]

I want to figur out how to move this forward. It seems like eclint could improve the consistency and thus readability of our source formatting.

I am going to first land this in a feature branch called add-eclint. Then any of us can contribute to it and it will be a little easier to work on it from the CLI. I still can't look at the files tab in this PR. It just won't load for me when JAWS is running.

Then, before merging, I want to make sure that:

1. eclint modifies only html, js, css, and md files. If possible, we would ensure that by asking it to explicitly only do that to guarantee it never touches any other type of file added to the repo, regardless of what type it has.
2. Other than the config, the only changes in the add-eclint branch are whitespace changes.

Just slap me if I am being overly conservative or careful. I just feel like any change that touches 144 files and every file checked in down the road deserves an extra bit of scrutiny. I want to know what I am doing :).