google / addlicense

A program which ensures source code files have copyright license headers by scanning directory patterns recursively
Apache License 2.0
716 stars 169 forks source link

add exclude path feature #70

Closed Shikugawa closed 3 years ago

Shikugawa commented 3 years ago

This PR introduces to skip check while filePath walk.

laurentsimon commented 3 years ago

@mco-gh @TomerAberbach do you have cycles to review this PR? We need support for this for Google's scorecard project. How can we be unblocked?

laurentsimon commented 3 years ago

@dlorenc whom shall we talk to have this PR reviewed/merged?

mco-gh commented 3 years ago

I can take a look later today.

On Wed, 28 Jul 2021 at 02:01, laurentsimon @.***> wrote:

@dlorenc https://github.com/dlorenc whom shall we talk to have this PR reviewed/merged?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/addlicense/pull/70#issuecomment-887931390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFAXF3MKMB7II7QRPUFFW3TZ5JF5ANCNFSM42IWOGOA .

-- Marc Cohen Web: mco.dev Email: @.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit

Q: Why is this email three sentences or less? A: three.sentenc.es

mco-gh commented 3 years ago

Left comments.

On Wed, 28 Jul 2021 at 08:59, Marc Cohen @.***> wrote:

I can take a look later today.

On Wed, 28 Jul 2021 at 02:01, laurentsimon @.***> wrote:

@dlorenc https://github.com/dlorenc whom shall we talk to have this PR reviewed/merged?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/addlicense/pull/70#issuecomment-887931390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFAXF3MKMB7II7QRPUFFW3TZ5JF5ANCNFSM42IWOGOA .

-- Marc Cohen Web: mco.dev Email: @.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit

Q: Why is this email three sentences or less? A: three.sentenc.es

-- Marc Cohen Web: mco.dev Email: @.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit

Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris commented 3 years ago

gah, I did it again! I started working on a feature before realizing there is another PR in flight. Well in any event, it may be worth a look, since I ended up taking a bit more of a flexible option by using https://github.com/bmatcuk/doublestar. This allows us to deprecate the existing -skip which is pretty limited in only supporting file extensions.

You can see the implementation here, and the test cases give a pretty clear example of what kinds of patterns would be possible: https://github.com/willnorris/addlicense/commit/6ac0919f1dcf4c714ba0b8a6ffc16011bf11c752. This implementation may still suffer from the relative vs absolute path issue discussed here. I haven't looked closely enough yet to see if you all settled on a solution to that yet.

mco-gh commented 3 years ago

I like this implementation. I'm less concerned about abs/rel paths than the fact that #82 doesn't handle dirs, only files. Your PR handles both so I'm inclined to use it. Give a little time to review before merging.

On Thu, 29 Jul 2021 at 00:54, Will Norris @.***> wrote:

gah, I did it again! I started working on a feature before realizing there is another PR in flight. Well in any event, it may be worth a look, since I ended up taking a bit more of a flexible option by using https://github.com/bmatcuk/doublestar. This allows us to deprecate the existing -skip which is pretty limited in only supporting file extensions.

You can see the implementation here, and the test cases give a pretty clear example of what kinds of patterns would be possible: willnorris@ 6ac0919 https://github.com/willnorris/addlicense/commit/6ac0919f1dcf4c714ba0b8a6ffc16011bf11c752. This implementation may still suffer from the relative vs absolute path issue discussed here. I haven't looked closely enough yet to see if you all settled on a solution to that yet.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/addlicense/pull/70#issuecomment-888695051, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFAXF5XGHNVBGBOVXUXMQ3T2CKEVANCNFSM42IWOGOA .

-- Marc Cohen Web: mco.dev Email: @.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit

Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris commented 3 years ago

It's also worth noting, and I only realized it later, that doublestar requires go1.16 because it's using the io/fs package. In #81, I have it testing against the last two minor releases, which would include 1.15. That's what we do for google/go-github, and what the Go project itself supports. Go typically releases at the end of July, so go1.17 should drop any day now. In that case, we might wait until go1.17 releases before adding in doublestar, if that's the approach you'd like to go with. Then we can continue to support the last two minor releases.

mco-gh commented 3 years ago

Works for me.

On Thu, 29 Jul 2021 at 16:02, Will Norris @.***> wrote:

It's also worth noting, and I only realized it later, that doublestar requires go1.16 because it's using thew io/fs package. In #81 https://github.com/google/addlicense/pull/81, I have it testing against the last two minor releases, which would include 1.15. That's what we do for google/go-github, and I think what the Go project itself supports (though I'm having trouble finding the docs). Go typically releases at the end of July, so go1.17 should drop any day now. In that case, we might wait until go1.17 releases before adding in doublestar, if that's the approach you'd like to go with. Then we can continue to support the last two minor releases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/addlicense/pull/70#issuecomment-889221630, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFAXFZXGNCIJ7STWADXWQTT2FUR3ANCNFSM42IWOGOA .

-- Marc Cohen Web: mco.dev Email: @.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit

Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris commented 3 years ago

alternate implementation of this feature was added in #84