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

Pass filenames to only lint changed files in pre-commit. #7433

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Aug 4, 2020

Summary

Pre-commit is still linting all files, rather than just changed files.

Makes pre-commit pass filenames again to make this happen.

Reviewer guidance

Does Travis still pass?


Contributor Checklist

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

Testing:

  • 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

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 rtibbles added the TODO: needs review Waiting for review label Aug 4, 2020
@rtibbles rtibbles added this to the 0.14.0 milestone Aug 4, 2020
@rtibbles rtibbles requested a review from jonboiser August 4, 2020 00:59
@jonboiser
Copy link
Contributor

jonboiser commented Aug 4, 2020

How are you running pre-commit, or how is it run on Travis?

When I run pre-commit run --all-files and add verbose: true to that hook, it makes it seem as if it this is equivalent to running yarn run lint-frontend:format once, with the current config.

@jonboiser
Copy link
Contributor

Oh, I see. If you use pre-commit after each commit you're not running it with the --all-files flag like it's used in Travis.

@jonboiser
Copy link
Contributor

I think my assumptions about how this all worked in #7401 are wrong. It's more like if you want to use pre-commit as part of your git workflow, you want the hook that just runs the changed files into the kolibri-tools lint command.

But in the context of Travis CI, I would prefer to have no filenames passed into the command so that the logs that result during a failure are less verbose. I don't know if you can have both of these.

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #7433 into release-v0.14.x will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted Files Coverage Δ
kolibri/core/content/utils/check_schema_db.py 91.30% <0.00%> (-8.70%) ⬇️

Co-authored-by: Jonathan Boiser <jonboiser@users.noreply.github.com>
@rtibbles
Copy link
Member Author

rtibbles commented Aug 4, 2020

Yeah - although I think we don't get as messy output from our other hooks, right? So if it's just the linting command that is problematic, we can fix that up to try to make something less messy.

Do you have a specific example of the messy output that we could look at more to try to make this better?

@jonboiser
Copy link
Contributor

Something like this is what I was trying to fix earlier:

https://travis-ci.org/github/learningequality/kolibri/jobs/711456164#L1082

At Line 1082 it logs the full command, which goes on for a few screenfuls of file paths, then finally to the actual linting error.

@indirectlylit
Copy link
Contributor

indirectlylit commented Aug 4, 2020

relevant recent PR: #6583

@rtibbles
Copy link
Member Author

rtibbles commented Aug 4, 2020

Hrm - do other linting hooks suffer from the same issue, or is it just ours?

@jonboiser
Copy link
Contributor

The 'import order' output is very concise when it "fails". Maybe we can see what is different about these community plugins to see if we can make the linting hook less verbose. But I'll merge this for now to improve people's git workflows.

Trim Trailing Whitespace.................................................Passed
Flake8...................................................................Passed
Check Yaml...........................................(no files to check)Skipped
Check for added large files..............................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Reorder python imports...................................................Failed
hookid: reorder-python-imports

Files were modified by this hook. Additional output:

Reordering imports in kolibri/core/api.py <- that's it, it doesnt output anything else

Linting of JS, Vue, SCSS and CSS files...................................

@jonboiser jonboiser merged commit c27e76d into learningequality:release-v0.14.x Aug 4, 2020
@jonboiser jonboiser removed the TODO: needs review Waiting for review label Aug 4, 2020
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.

None yet

3 participants