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

Add lint --only <files...> option #903

Closed
wants to merge 2 commits into from

Conversation

Wuestengecko
Copy link

@Wuestengecko Wuestengecko commented Feb 5, 2024

Here's my second shot at #578. I went with the --only option that @stephanlachnit proposed, as I found it a bit easier to implement than a new subcommand. It should also be fairly simple to extend it with an --exclude option later.


This option restricts the generated report to only the files listed on the command line. It is useful for example in the pre-commit hook, in order to not fail the entire commit if there are some issues in files that aren't supposed to be committed yet anyways.

This option is implemented by checking all files that we know about as usual, but only reporting on (and failing for) issues in files listed.

We have to check all files, otherwise we wouldn't be able to catch certain types of errors reliably or at all. Specifically, the "unused licenses" check would often raise false-positive errors: If we only look at the to-be-committed files, and those don't use a particular license, we would fail the check even if another file uses it (which was already committed before, but not touched during the new commit).

This does however mean that untracked files could cause false-negatives, but this was already the case before, and will (still) be caught by CI.

One alternative to this approach is to not check for unused licenses at all if doing such a partial check, but this would mean that downstream CI scripts would have to be adjusted to explicitly run a full reuse lint instead of running the pre-commit hook over all repo files. However, this would only lead to a lot of complications without any meaningful benefit.


Annoyingly pylint now complains about "too many locals", because I introduced a new argument to ProjectReport.generate, which ruins the otherwise perfect score. To avoid this, I have renamed the missing_license and bad_license loop variables to license_name, because they're both just strings anyways (from a type checker's perspective), and IMO license_name is still explicit enough. It's a separate commit, so if you don't want that, we can just force-push it away and do something else instead :)

This option restricts the generated report to only the files listed on
the command line. It is useful for example in the pre-commit hook, in
order to not fail the entire commit if there are some issues in files
that aren't supposed to be committed yet anyways.

This option is implemented by checking all files that we know about as
usual, but only reporting on (and failing for) issues in files listed.

We have to check all files, otherwise we wouldn't be able to catch
certain types of errors reliably or at all. Specifically, the "unused
licenses" check would often raise false-positive errors: If we only look
at the *to-be-committed* files, and those don't use a particular
license, we would fail the check even if another file uses it (which was
already committed before, but not touched during the new commit).

This does however mean that untracked files could cause false-negatives,
but this was already the case before, and will (still) be caught by CI.

One alternative to this approach is to not check for unused licenses at
all if doing such a partial check, but this would mean that downstream
CI scripts would have to be adjusted to explicitly run a full `reuse
lint` instead of running the pre-commit hook over all repo files.
However, this would only lead to a lot of complications without any
meaningful benefit.
Reuse the same loop variable name across two consecutive loops. The
loops are very short and have a very similar purpose, and doing this
makes pylint happy.
@Wuestengecko
Copy link
Author

Hey! It's been quite a while since I opened this PR. Has anyone had a chance to look at it already? It seems that community interest is definitely there, and I don't want to see it just die completely.

@carmenbianca
Copy link
Member

Hi! Sorry, I should have followed up on this earlier.

In fact yes, there is interest for this, and things have been happening to facilitate this PR. @nicorikken wrote #956, which would be an excellent output format for this.

In any case, the way of implementing this feature will not be with an --only flag to lint. It is a deliberate design decision that lint takes (almost) no arguments. This will be documented in CONTRIBUTING.md one day.

Instead, it will be a separate subcommand lint-file (or called some such).

After #956 is merged, I can get to work on morphing this PR into something else. However, because time is finite, I won't really be able to get to that until end-May-start-June.

If you want to expedite this PR, you can attempt to do the above morphing after #956 is merged. #863 contains a commit that introduces convert-dep5, which may be a good reference on how to add subcommands.

Thanks again, and sorry for the lack of communication.

@Wuestengecko
Copy link
Author

Ah, that's good to know, thanks for the info. I'll try to change this into a subcommand one of these days, but I can't guarantee that's going to happen before you get to it :)

@carmenbianca
Copy link
Member

Closed by #1055, thank you!

@Wuestengecko
Copy link
Author

Great, thank you too!

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