Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linter: Catch "no undef" rules #1429

Closed
wants to merge 4 commits into from
Closed

Linter: Catch "no undef" rules #1429

wants to merge 4 commits into from

Conversation

spectranaut
Copy link
Contributor

I'm just curious if there is support for this change! Now that we are using "no strict" it would be nice to catch undefs in the linter, rather than in edge cases in the application.

This PR will fail in the CI for two reasons (for now):

  1. There are some undef's fixed in Add "use strict" and lint rule #1416 (unmerged)
  2. The js files in the example reference globals defined in other files. For eslint to pass, we will have to warn eslint about these variables using this method. I wanted to make sure there was support for the change before doing the work.

@spectranaut spectranaut marked this pull request as draft June 25, 2020 18:39
@spectranaut spectranaut requested a review from smhigley June 25, 2020 18:39
.eslintrc.json Outdated
},
"parserOptions": {
"ecmaVersion": 6
},
"rules": {
"no-unused-vars": 0,
"no-undef": 0,
"no-undef": "error",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this line can just get deleted since it's in the "recommended" rules https://eslint.org/docs/rules/no-undef

.eslintrc.json Outdated
@@ -1,14 +1,16 @@
{
"extends": "eslint:recommended",
"env": {
"browser": true
"browser": true,
"node": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think node should be on, since it might not flag things the examples. The scripts folder already has it one since those are nodejs scripts

@spectranaut spectranaut marked this pull request as ready for review September 8, 2020 23:35
@spectranaut
Copy link
Contributor Author

Whoops I forgot about this PR @nschonni just got to your comments!

@github-actions
Copy link
Contributor

Examples without any regression tests:

button/button_idl.html
dialog-modal/alertdialog.html


Examples missing some regression tests:

combobox/combobox-autocomplete-both.html:
    combobox-id
combobox/combobox-autocomplete-list.html:
    combobox-id
combobox/combobox-autocomplete-none.html:
    combobox-id
combobox/grid-combo.html:
    textbox-key-down-arrow
    textbox-key-up-arrow
dialog-modal/datepicker-dialog.html:
    textbox-aria-describedby
menu-button/menu-button-actions-active-descendant.html:
    menu-up-arrow
    menu-down-arrow
    menu-character
spinbutton/datepicker-spinbuttons.html:
    spinbutton-down-arrow
    spinbutton-up-arrow
    spinbutton-page-down
    spinbutton-page-up
    spinbutton-home
    spinbutton-end
toolbar/toolbar.html:
    toolbar-tab
    toolbar-right-arrow
    toolbar-left-arrow
    toolbar-home
    toolbar-end
    toolbar-toggle-esc
    toolbar-toggle-enter-or-space
    toolbar-radio-enter-or-space
    toolbar-radio-down-arrow
    toolbar-radio-up-arrow
    toolbar-button-enter-or-space
    toolbar-menubutton-enter-or-space-or-down-or-up
    toolbar-menu-enter-or-space
    toolbar-menu-down-arrow
    toolbar-menu-up-arrow
    toolbar-menu-escape
    toolbar-spinbutton-down-arrow
    toolbar-spinbutton-up-arrow
    toolbar-spinbutton-page-down
    toolbar-spinbutton-page-up
    toolbar-checkbox-space
    toolbar-link-enter-or-space
    toolbar-aria-controls
    toolbar-button-aria-pressed
    toolbar-button-aria-hidden
    toolbar-radiogroup-role
    toolbar-radiogroup-aria-label
    toolbar-radio-role
    toolbar-radio-aria-checked
    toolbar-radio-aria-hidden
    toolbar-button-aria-disabled
    toolbar-menubutton-aria-label
    toolbar-menubutton-aria-haspopup
    toolbar-menubutton-aria-controls
    toolbar-menubutton-aria-expanded
    toolbar-menu-role
    toolbar-menu-aria-label
    toolbar-menuitemradio-role
    toolbar-menuitemradio-aria-checked
    toolbar-menuitemradio-tabindex
    toolbar-spinbutton-role
    toolbar-spinbutton-aria-label
    toolbar-spinbutton-aria-valuenow
    toolbar-spinbutton-aria-valuetext
    toolbar-spinbutton-aria-valuemin
    toolbar-spinbutton-aria-valuemax


Examples documentation table rows without data-test-ids:

dialog-modal/alertdialog.html
    "Keyboard Support" table(s):
       Tab
       Shift + Tab
       Escape
       Command + S
       Control + S
    "Attributes" table(s):
       alertdialog
       aria-labelledby=IDREF
       aria-describedby=IDREF
       aria-modal=true
       alert

SUMMARTY:

  55 example pages found.
  2 example pages have no regression tests.
  8 example pages are missing approximately 61 out of approximately 775 tests.

ERROR - missing tests:

  Please write missing tests for this report to pass.

@@ -1,5 +1,8 @@
{
"extends": "../.eslintrc.json",
"env": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternately, this file could be deleted, and the config hoisted to an overrides section in the root config like https://eslint.org/docs/user-guide/configuring#specifying-processor

@spectranaut
Copy link
Contributor Author

Hi @mcking65 you asked to understand the options a bit better here: If we want to merge this js lint error (catching any new undefined variables), we would have to edit 30 or so files to add a eslint comment specifically listing which globals they should not error on. A lot of those undefs come from the old way APG example code was structured, in multiple files, but Jon has been changing that example by example. It will take a while for all those rewrites to be completed, which is why I want to just go through and add the ignore comment to all these files for now!

@spectranaut
Copy link
Contributor Author

Closing this PR in favor of @nschonni's: https://github.com/w3c/aria-practices/pull/1610/files

@zcorpan zcorpan deleted the error-on-no-undef branch May 31, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants