-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support simple patterns for codemod include/exclude #458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the request to log bad excluded was yours and you see the lengths I had to go to get it there so it's kinda nice that this code is simpler without it. I do actually think the logging was useful but up to you.
Approving but I think small change is needed.
src/codemodder/registry.py
Outdated
base_codemods.pop(codemod.id, None) | ||
if bool(sast_only) != bool(codemod.origin == "pixee"): | ||
base_codemods[codemod.id] = codemod | ||
|
||
# Remove duplicates and preserve order | ||
return list(dict.fromkeys(base_codemods.values())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is so subtly changed so I'm going off passing unit tests but I think you can remove this list(dict.from_keys...) since that was necessary when base_codemods did this:
base_codemods[codemod.id] = codemod
base_codemods[codemod.name] = codemod
but since that was only necessary to log an excluded codemod and that's been removed, you can remove this duplication removal.
not sast_only and codemod.origin == "pixee" | ||
if ( | ||
codemod.id in names | ||
or (codemod.origin == "pixee" and codemod.name in names) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this line accomplish this issue for excluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think it does for the exclude case but I think we'd want to add more tests so we can keep it as a separate issue.
e7fd202
to
31d88f2
Compare
Quality Gate passedIssues Measures |
Yes sorry I see that this was complicated and I'm just not sure it's necessary so I was able to simplify it. |
Overview
Support simple patterns for codemod include/exclude CLI flags
Description
sast_only
but I left it as-is for this PRCloses #316