Discussion item: make sure we are using the very best angular linting rules

[Jane Sandberg]

We can enable or disable tslint rules to help keep our Angular code quality high, and to help educate folks new to writing Angular about best practices.

Personally, I like enabling this one -- no-magic-numbers -- although it is not helpful for .spec.ts files.

We can also add custom rules that others have created. For example, I like some of these: https://cartant.github.io/rxjs-tslint-rules/ (especially rxjs-no-nested-subscribe and rxjs-finnish)

We could also write some custom rules of our own.

[Bill Erickson (berick)]

Thanks, Jane. +1 to your specific suggestions.

[Jane Sandberg (sandbergja)]

Here are a few more useful ones (which don't all pass at the moment):

* template-accessibility-alt-text
* template-accessibility-label-for
* template-accessibility-valid-aria

[Jane Sandberg (sandbergja)]

Here is collab branch: collab/sandbergja/lp1850473_angular_lint_rules

I just threw in a commit that enables all these lint rules. As discussed earlier today, we can work on correcting the issues that the new lint rules uncover.

[Jane Sandberg (sandbergja)]

Noting that tslint is now deprecated. Angular 11 includes tools for migrating from tslint to eslint (https://github.com/angular-eslint/angular-eslint)

As far as I can tell, that means that we will have access to a ton of new rules from eslint: https://eslint.org/docs/rules/

However, some of the codelyzer rules I was most excited about -- template-accessibility-alt-text and template-accessibility-label-for -- have not (yet?) been ported over to eslint.

[Jane Sandberg (sandbergja)]

I'm pretty sure I inadvertently turned most of our linting rules off in bug 1959048 :-(

Grabbing this bug to get them turned on again, enable some rxjs linting, and adding no-magic-numbers to non-spec files. The accessibility linting rules are covered by bug 2017793, so I am not including them in this project.

[Jane Sandberg (sandbergja)]

Here is just a humongous pull request, turning back on the eslint versions of our old codelyzer rules, and adding the aforementioned new rules: user/sandbergja/lp1850473_ng_lint

Here is a link: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1850473_ng_lint

[Bill Erickson (berick)]

Sign-off pushed:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1850473-eslint-expansion

Given the scope and size of these commits, I'm eager to get them merged sooner than later. I'll leave a little time for additional review and hope to merge mid next week.

Thanks Jane!

[Galen Charlton (gmc)]

Grabbing this for further review. Also requesting that notification be sent to the development mailing list, as this patch will likely cause merge conflicts for a good many of the extant Angular pull requests.

[Mike Rylander (mrylander)]

Better living through coding style automation!

However, with our traditional "release feature stampede" coming relatively soon, I wonder if we might want to err in the other direction WRT timing. In particular, I think doing a linter driven change-all-the-spacing commit might be best during the feature freeze or beta/RC phase might cause less total pain. That would also allow us to test and publicize the impact, and alert the dev audience a bit more explicitly, before we potentially blow up (especially) branches that have been waiting patiently for review and commit.

As for the changes themselves, I'd guess based on scanning down through the big commit that 90%+ of the changes should be trivially conflicting to the point that git really should be able to handle it on its own. However, I did see at least a few changes that are more than whitespace cleanup and import optimizations. Of note are some changes from "==" to "===" that should probably be considered carefully (as "==" can be perfectly reasonable, and even desirable), and I have to say that the output for switch/case formatting and indent folding is almost exactly the opposite of what I would prefer to see, personally.

Regardless, clean-code automation will be an improvement when it's eventually applied, so thank you Jane for your work on this!

[Stephanie Leary (stephanieleary)]

I like the idea of doing this during feature freeze.

We could take the opportunity to clean up the accessibility lint errors noted in bug 2017793, since that will involve another large set of files with relatively minor changes.

[Jane Sandberg (sandbergja)]

Okay, my new plan is to:

* redo this work next week, after feature freeze
* configure eslint to prefer a different switch/case format (that one made me cringe a little too, Mike!)
* only change == to === in code that is under some type of automated test. I will note that this rule was in the old codelyzer implementation, so it is not new, just... it's been inactive for a while?

My outstanding questions are:
* the discussion on bug 2017793 proposes that we wait until Angular 16 before adding accessibility rules. I'm not opposed to adding them as part of this work, but just want to confirm that it's what we prefer.
* what more publicity is required?

[Stephanie Leary (stephanieleary)]

I would prefer that we add the accessibility rules in their verbose form for now and switch to the more concise ones when we upgrade Angular. I plan on cleaning up the various issues listed in bug 2017793, and can coordinate timing so that we get all our syntactical annoyances out of the way at once.

[Jane Sandberg (sandbergja)]

Okay, another branch: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/sandbergja/lp1850473_ng_lint-2

This takes care of the more than 7,000 issues it found in our .ts files. There are 534 remaining issues in the html files, mainly from the accessibility rules. Please feel very free to push additional commits to this branch, and to rebase it to keep it fresh.

[Stephanie Leary (stephanieleary)]

I've pushed several commits to the collab branch to deal with the accessibility issues. Most of them involved link/button issues. We're down to 72, all of which are form label problems except for two files with interaction errors that will take some more work to clean up:

staff/reporter/simple/sr-field.component.html -- there are numerous button errors here that we fixed during the work on Angular Reports (forthcoming very soon), so I'd like to leave this file alone for now rather than try to isolate those changes.

staff/admin/local/shelving_location_groups/shelving_location_groups.component.html -- the Shelving Locations admin page has a ton of accessibility errors, and I've opened a bug for that, bug 2042879. (Related, Shelving Locations Order is not great but at least passes tests now; bug 2043617 will clean that up.)

Other errors I have silenced in the latest commit:

staff/login.component.html -- aria-description is invalid. True, but only because the relevant W3C committee is lagging on adding it to the official ARIA spec. It's supported in all browsers, and I think we can just ignore this error until the spec gets updated.

share/grid/grid-body.component.html -- has to do with the single click action on grid rows. We do have a keyboard-accessible alternative to all these functions and there's no straightforward way to work around this.

staff/sandbox/sandbox.component.html -- all that's left are the deliberately bad button examples. We'll have to decide whether to move those to the wiki; for now I've disabled the relevant rules in this section.

[Jane Sandberg (sandbergja)]

Stephanie and I discussed on IRC to defer the remaining accessibility rule violations to a future ticket, to get these in.

Thanks, Stephanie! I've signed off on your changes, rebased, and I think this is ready for more review (and hopefully a merge before it goes stale).

To exercise this new configuration, you can do something like the following:

1. cd to the eg2 directory and run `ng lint`
2. Confirm that it says that everything's fine
3. Add a problem. For example, you could add to src/app/app.module.ts the following line:

var cat = "tuxedo cat";

4. Run `ng lint` again.
5. Confirm that it is complaining, and advising you to use "const" or "let" instead of "var".

[Stephanie Leary (stephanieleary)]

Thanks, Jane! We can put the remaining rules on bug 2017793 rather than opening another one.

[Jane Sandberg (sandbergja)]

Rebased against today's main.

[Terran McCanna (tmccanna)]

The patch applies cleanly and the testing steps in comment #15 work well.

[Jane Sandberg (sandbergja)]

Rebased this today

[Terran McCanna (tmccanna)]

Discussed and reviewed again in Code Review meeting and merged for inclusion in 3.12. This may affect branches that currently have pullrequests that are not yet merged, as well as branches that are still under development. A follow-up message to the Dev list is going out.

[Jane Sandberg (sandbergja)]

I just pushed a small follow-up to main and rel_3_12: letting nightwatch know that a button had been changed to a link.