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

Improve clarity of ESLint failure reports #252

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Aug 4, 2022

As suggested by Brendan, I've added two things here:

  • An INFO line to indicate that ESLint reported JS errors
  • A copy of the offending with a caret pointing to the exact failure

I have also taken out the source from the brackets to make the line length shorter.
If there is no column, then the whole line is printed without any caret underneath.

Before:

➜  73808 git:(MDL-75365-master) ✗ php local/ci/mustache_lint/mustache_lint.php -f=`pwd`/lib/templates/welcome.mustache --basename=`pwd` -v=https://html5.validator.nu
/Users/nicols/Sites/public_html/73808/lib/templates/welcome.mustache - WARNING: ESLint error []: Parsing error: Unexpected token foo ( let foo = 'bar'; ), Line: 2 Column: 5

After:

➜  73808 git:(MDL-75365-master) ✗ php local/ci/mustache_lint/mustache_lint.php -f=`pwd`/lib/templates/welcome.mustache --basename=`pwd` -v=https://html5.validator.nu
/Users/nicols/Sites/public_html/73808/lib/templates/welcome.mustache - INFO: ESLint reported JavaScript errors
/Users/nicols/Sites/public_html/73808/lib/templates/welcome.mustache - WARNING: ESLint error []: Parsing error: Unexpected token foo, Line: 2 Column: 5
/Users/nicols/Sites/public_html/73808/lib/templates/welcome.mustache - WARNING: let foo = 'bar';
/Users/nicols/Sites/public_html/73808/lib/templates/welcome.mustache - WARNING:     ^

@brendanheywood
Copy link

🥳

@stronk7
Copy link
Member

stronk7 commented Aug 7, 2022

Oh, just came here after looking to #251 . Is this an alternative to it?

In any case, it seems that some tests need to be amended to fulfil the new expected output (and also, to rebase this on top of current master).

Ciao :-)

@andrewnicols andrewnicols force-pushed the mdlsite-6748-improve_eslint_clarity branch from 13298f0 to 404561b Compare August 8, 2022 08:26
@andrewnicols
Copy link
Contributor Author

Oh, just came here after looking to #251 . Is this an alternative to it?

No, this change follows from that one and is based on it.
The other change is obvious and requires little discussion, whilst this one is more contentious so I split tem.

In any case, it seems that some tests need to be amended to fulfil the new expected output (and also, to rebase this on top of current master).

I amended existing tests which should cover this.

@andrewnicols andrewnicols force-pushed the mdlsite-6748-improve_eslint_clarity branch from 404561b to c79c408 Compare September 27, 2022 04:29
@andrewnicols andrewnicols self-assigned this Sep 27, 2022
@stronk7 stronk7 force-pushed the master branch 4 times, most recently from 7c835b9 to 0126fe3 Compare March 9, 2023 17:38
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.

3 participants