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

Allow excluding files from search #94

Closed
ihrwein opened this issue Oct 1, 2021 · 6 comments · Fixed by #97
Closed

Allow excluding files from search #94

ihrwein opened this issue Oct 1, 2021 · 6 comments · Fixed by #97
Labels
type: feature New feature

Comments

@ihrwein
Copy link

ihrwein commented Oct 1, 2021

Problem

First of all, thank you for this really useful project.

Shisho finds matches in files which are auto-generated or I don't have control over them (vendored). It's not feasible to fix the reported issues in these files.

I.e. consider this policy file for Golang:

version: '1'
rules:
  - id: 'deny-fmt-println'
    language: go
    message: |
      Avoid calling fmt.Println, as it can make log output messy. Use logging instead.
    pattern: |
      fmt.Println(:[_])
  - id: 'deny-panic'
    language: go
    message: |
      Avoid calling panic() manually. Return the error instead.
    pattern: |
      panic(:[_])

Both rules will likely find matches in the vendor folder. Some libraries generate code with panic() calls in it: https://github.com/99designs/gqlgen/blob/1a0b19feff6f02d2af6631c9d847bc243f8ede39/example/chat/generated.go#L2028

Possible Solutions

  • generated code & vendor folder: I'd like to exclude it from the search, perhaps via CLI flag or extending the rule file with file excludes.
  • ignoring false positives: Sometimes we want to silence the linter in particular cases. We could use some comments and either match against those comments in constraints (manually) or make shisho aware of a convention: panic("foo") // shisho:ignore deny-panic

Additional Notes

@lmt-swallow
Copy link
Member

@ihrwein Thank you soooo much!

generated code & vendor folder: I'd like to exclude it from the search, perhaps via CLI flag or extending the rule file with file excludes.

Yeah, I believe this kind of feature is neccesary to Shisho. For CLI flag, how do you feel about adding --exclude <path to ignore> flag? Here's a possible command example:

shisho check --exclude "vendor/*" --exclude "any/other/dir/*.yaml" <blah> <blah>

On the other hand, I feel that rule files should not include file paths in order to keep portability of the rules. Instead we can add a feature to handle something like .gitignore file:

any/other/dir/*
vendor/*

I'd like to get your thoughts on these designs! 😃

ignoring false positives: Sometimes we want to silence the linter in particular cases. We could use some comments and either match against those comments in constraints (manually) or make shisho aware of a convention: panic("foo") // shisho:ignore deny-panic

I totally agree with you. It will be awesome if Shisho has a feature to suppress reports with comments as you propose. I'll try to implement this in 3 or 4 days.

@lmt-swallow lmt-swallow added the type: feature New feature label Oct 1, 2021
@ihrwein
Copy link
Author

ihrwein commented Oct 1, 2021

Hello @lmt-swallow, thanks for the super quick turnaround :)

Regarding excluding files, I like both of your proposals. The declarative ,gitignore style approach because we can version control what files need to be excluded, so shisho can be run as simply as shisho check. This way no makefiles are needed. I'm not sure what other config parameters are expected - if there are some more, the .gitignore format might be too restrictive on the long run.

Having a way to override the excludes from the CLI also looks useful. I like your proposal - it supports many excludes and globs. I can imagine this is the simpler option to start with (people can use Makefiles), then as more feedback comes the config file can be added?

@lmt-swallow
Copy link
Member

lmt-swallow commented Oct 2, 2021

Hello @ihrwein . Thank you for your comment!

I'm not sure what other config parameters are expected - if there are some more, the .gitignore format might be too restrictive on the long run.

I have no idea about whether we need a more fine-grained control for each directory, but I guess you're right and rubocop-style approach might work better rather than .gitignore-style approach 😃

I can imagine this is the simpler option to start with (people can use Makefiles), then as more feedback comes the config file can be added?

Looks great! I'll try to add --exclude CLI flags first.

@lmt-swallow
Copy link
Member

Hello @ihrwein, I added --exclude flag to shisho check and shisho find command in v0.4.0 (#97). Could you check the feature works as you expected? 😃

And, as discussed above, we still need a feature to ignore certain files in a .gitignore-like or .robocop.yaml-like manner. I've created #98 to track the process on this feature.

@ihrwein
Copy link
Author

ihrwein commented Oct 8, 2021

Hello @lmt-swallow. It works like a charm, thank you!

@lmt-swallow
Copy link
Member

You're welcome. I'm happy to hear that! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants