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 glob support to ignores #76

Closed
wants to merge 14 commits into from
Closed

Add glob support to ignores #76

wants to merge 14 commits into from

Conversation

hanneskaeufler
Copy link

@hanneskaeufler hanneskaeufler commented May 28, 2022

As is on the v2 wishlist and was mentioned in #35 and #56 (comment), globbing is currently not allowed for the ignore key.

This is a first try at implementing this.

  • Performance implications?
  • Documentation?
  • Haven't found a changelog

Performance

All measurements done on an 2020 M1 MacBook Air with 8GB of ram.
Using hyperfine, I ran hyperfine './ls-lint' with the current local build of ls-lint as well as with master. I did a couple variations, based on the example config:

build config result
this branch with node_modules in ignore Time (mean ± σ): 16.0 ms ± 0.8 ms [User: 7.4 ms, System: 8.7 ms]
this branch with '**/node_modules' in ignore Time (mean ± σ): 282.4 ms ± 21.8 ms [User: 71.7 ms, System: 210.1 ms]
master with node_module in ignore Time (mean ± σ): 15.9 ms ± 0.7 ms [User: 7.3 ms, System: 8.7 ms]

This kind of confirms what makes some sense: If you use a glob in ignores, depending on the size of your project, you pay a significant penalty in performance. If you don't use any globs, you don't pay measurably.

I was confused when first reading
through the test output as to what
is framework output and what is test
output. I think more verbose test names
helps allow newcomers scan the list and
make more sense out of it.
In contrast to a method, because, apparently
go generics doesn't allow generic methods.
Meaning I hopefully can de-duplicate the two
implementations
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #76 (901d6e1) into master (3d00746) will increase coverage by 1.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   66.89%   67.94%   +1.05%     
==========================================
  Files          13       13              
  Lines         441      443       +2     
==========================================
+ Hits          295      301       +6     
+ Misses        129      127       -2     
+ Partials       17       15       -2     
Impacted Files Coverage Δ
config.go 76.19% <100.00%> (+2.38%) ⬆️
linter.go 74.10% <100.00%> (+2.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d00746...901d6e1. Read the comment docs.

@loeffel-io
Copy link
Owner

Great! Thanks for the PR! I am currently on vacation and get back to you asap

@RaisinTen
Copy link

Gentle ping @loeffel-io, just wanted to make sure that you didn't forget about this PR 😅

@MagnusHJensen
Copy link

MagnusHJensen commented Aug 1, 2022

What's the status on this? Feels very high priority.

@hanneskaeufler hanneskaeufler marked this pull request as ready for review September 3, 2022 20:13
@hanneskaeufler
Copy link
Author

@loeffel-io friendly ping :) Anything I can do to move this along? Thanks!

@GeorgeTaveras1231
Copy link

Would be great to get this merged and published. There doesn't seem to be a good way to ignore certain folder names in arbitrary locations in a repo. I have a monorepo with multiple tests folders that I would like to ignore.

@loeffel-io
Copy link
Owner

Hey folks,

sorry that this takes so long - the support will come! This will come together with https://github.com/github/super-linter/issues/2505#issuecomment-1263886322 as soon i have enough mind capacity

@ProjectBarks
Copy link

@loeffel-io Is there anything we can do to help get this merged? Seems like it has been stale for a bit.

Copy link
Owner

@loeffel-io loeffel-io left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @hanneskaeufler!

The tests are not meaningful enough imo - we should add ignore mid path tests and some more ignore variations.

is there a reason for removing the globIndex func from the config struct?

thanks!

@loeffel-io loeffel-io self-assigned this Jan 2, 2023
@dror-weiss
Copy link

ping from here as well; I need glob support in the ignore section for my monorepo 🙏🏻

@gbrandt1
Copy link

no support for ignore globs is a show-stopper for me.

@loeffel-io
Copy link
Owner

loeffel-io commented May 21, 2023

Very happy to announce that v2.0.0-beta.0 is now available: https://github.com/loeffel-io/ls-lint/releases/tag/v2.0.0-beta.0
Please see the changelog for any additional info: https://ls-lint.org/2.0/prologue/changelog.html#v2-0-0-beta

This feature (add glob support to ignores) is planned for v2.1
I will close this PR for now (there will be a new one)

@loeffel-io loeffel-io closed this May 21, 2023
@loeffel-io
Copy link
Owner

This is now supported with v2.1.0-beta.0 🎉 feel free to test it!
more infos: #119

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

Successfully merging this pull request may close these issues.

9 participants