add a "skip next line" pragma #367
Conversation
There was a problem hiding this comment.
Great turnaround on this!
Besides stylistic comments, you will also need to modify https://github.com/Yelp/detect-secrets/pull/356/files#diff-bad716776100dfb6fdad237381526e524042316894d806953446d39984aabe27R165 to inject context into the filter -- otherwise, this will run per plugin, which is not something we want.
There was a problem hiding this comment.
@nickiaconis, please also modify https://github.com/Yelp/detect-secrets/pull/356/files#diff-bad716776100dfb6fdad237381526e524042316894d806953446d39984aabe27R165 to include the context variable in there -- otherwise, this PR won't work.
| r'{}{} *pragma: ?{}{}[ -]secret.*?{}[ \t]*$'.format( | ||
| r'^[ \t]*' if nextline else r'[ \t]+', | ||
| start, | ||
| r'allowlist' if nextline else r'(allow|white)list', |
There was a problem hiding this comment.
I'm also fine if these are the same. Supporting whitelist is just for backwards compatibility, and I'm fine with keeping that backwards compatibility around for the nextline regex too.
There was a problem hiding this comment.
I don't mind one way or the other, but my thinking was that, b/c nextline is new, it doesn't have any backward compatibility to contend with. Thus, requiring allowlist for nextline prepares uses of it for the future removal of whitelist, whereas allowing both opens the possibility of further entrenching whitelist.
|
Totally missed the top comment and link to the diff in your previous review. Sorry about that. 😅 Just to double check, is this the line in question? https://github.com/Yelp/detect-secrets/blob/pre-v1-adding-verification/detect_secrets/core/scan.py#L165 It looks like GH can't scroll to a line in a diff that's not part of the diff context. |
|
Yup, that's the one. |
|
Hey @domanchi, just want to follow up on this. Is there anything left unresolved at this point? |
Co-authored-by: Aaron Loo <[email protected]>
|
I think that's everything, but just lmk if there's anything more to be done here. 🙂 |
|
Ah whoops. Did not mean to close this PR by cleaning up my branches. I will merge your changes manually. |
|
This has been merged: b73c473 |
|
Hello @domanchi ! When can we expect to see this change published? |
Some configuration files, like Java's
.propertiesfiles and Docker'sDockerfile, don't allow inline comments. This PR adds support for a "skip next line" pragma to solve this use case. For example:This should solve #319.