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

adds additional output to linting tool for formatting failure case #9691

Closed

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Sep 3, 2022

Summary

While working on learningequality/kolibri-design-system#358 I was confused why sometimes linting would fail without any actionable error:

~/Projects/le/design-system main* ⇡ 6s
❯ yarn lint
yarn run v1.22.18
$ kolibri-tools lint --pattern '**/*.vue' -i ,'**/node_modules/**','**/.nuxt/**','**/dist/**','**/lib/KIcon/precompiled-icons/**','**/lib/keen/**','**/docs/environment.js','**/docs/jsdocs.js'
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Running yarn lint-fix ended up reformatting a file, so it was something to do with prettier or eslint.

I believe this output should give more actionable information about why the lint command might be failing.

Reviewer guidance

  • Modify a file such that it might be reformatted by prettier, e.g. < div >hello</div>
  • Run lint-frontend without and without this patch
  • With the patch it should show what file is causing issues

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles
Copy link
Member

rtibbles commented Sep 6, 2022

Flagging that I may neglect to merge this in favour of current WIP to upgrade eslint and address #5410 - I'll see whether it conflicts with my current work or not.

@indirectlylit
Copy link
Contributor Author

indirectlylit commented Sep 6, 2022

sure, @MisRob or you can just make this update there and close this PR

@indirectlylit
Copy link
Contributor Author

Sorry for the noise @MisRob I see you filed the issue, and @rtibbles is working on the PR. my bad

@rtibbles
Copy link
Member

rtibbles commented Sep 6, 2022

OK, I think this was probably caused by either eslint or stylelint reformatting a file, but prettier not? Either way, I think this should be covered in my changes.

@MisRob
Copy link
Contributor

MisRob commented Sep 7, 2022

No worries, always happy to hear from you in any way, @indirectlylit ;)

@MisRob
Copy link
Contributor

MisRob commented Sep 7, 2022

Thanks for another contribution

@rtibbles
Copy link
Member

Superseded by #9698

@rtibbles rtibbles closed this Sep 27, 2022
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