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

Implement codemod registry #47

Merged
merged 5 commits into from
Sep 27, 2023
Merged

Implement codemod registry #47

merged 5 commits into from
Sep 27, 2023

Conversation

drdavella
Copy link
Member

@drdavella drdavella commented Sep 25, 2023

Overview

Move codemods to core_codemods package; implement codemod registry

Description

  • This PR moves the codemods to the core_codemods package and implements a codemod registry
  • There are a few major goals with this change:
    • Separate individual codemod implementation from the codemodder package itself
    • Move in the direction of "external" validation for codemods rather than using __init_subclass__ hooks
    • Enable future third-party codemod implementation/registration
  • The codemod registry is responsible for detecting and collecting all registered codemods
    • It also performs codemod validation and filtering based on ID
  • Codemod docs and semgrep configurations are now more tightly coupled with the codemods themselves
  • Codemods are registered with a namespace prefix that no longer needs to be hardcoded in codemodder
  • I was able to remove the __init_subclass__ validation for the "base" API but some additional work will be required before we can get rid of this in the "sugar" API
    • I still think that maybe these shouldn't be class-level attributes; hopefully this moves us a bit closer to that goal
  • This is a relatively big change but I also view it as just one step of incremental refactoring
    • I don't think the result is perfect yet but hopefully it moves in a good direction
    • I hope that eventually this will better enable us to clean up the codemod class hierarchy a bit

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Merging #47 (5afd594) into main (03a1d7e) will increase coverage by 0.04%.
The diff coverage is 97.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   95.82%   95.86%   +0.04%     
==========================================
  Files          38       39       +1     
  Lines        1413     1476      +63     
==========================================
+ Hits         1354     1415      +61     
- Misses         59       61       +2     
Files Coverage Δ
src/codemodder/__main__.py 96.93% <100.00%> (-0.24%) ⬇️
src/codemodder/cli.py 100.00% <100.00%> (ø)
src/codemodder/codemods/api/__init__.py 95.38% <ø> (-0.14%) ⬇️
src/codemodder/codemods/base_codemod.py 98.03% <100.00%> (+2.32%) ⬆️
src/codemodder/context.py 100.00% <100.00%> (ø)
src/codemodder/semgrep.py 96.29% <100.00%> (-0.93%) ⬇️
src/core_codemods/__init__.py 100.00% <100.00%> (ø)
src/core_codemods/django_debug_flag_on.py 100.00% <ø> (ø)
.../core_codemods/django_session_cookie_secure_off.py 98.30% <ø> (ø)
src/core_codemods/harden_pyyaml.py 100.00% <ø> (ø)
... and 15 more

@drdavella drdavella marked this pull request as ready for review September 25, 2023 20:33
codemods: list


class _CodemodContainer:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a slightly awkward construct but I might suggest that in the longer time this actually outlines the API we want for the BaseCodemod class. Eventually maybe this becomes an ABC that all codemods inherit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both CodemodCollection and CodemodContainer sound to me like containers of some sort, such as a list. What about making one of these inherit from list? Maybe that's a later thing, just a thought.

Also, how about renaming _CodemodContainer to CodemodMetadata or CodemodWrapper? Also I think this class in particular could be a dataclass. What clues me in to that is all the property methods, but some will have to be manual, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

More thoughts, since you have the idea of this class _CodemodContainer being the "real" BaseCodemod, what do you think is the distinction between the data a BaseCodemod class should store versus this new _CodemodContainer? I still feel they should be one class but I'm listening!

Copy link
Member Author

Choose a reason for hiding this comment

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

  • I did think about making CodemodCollection a list but I'm not sure whether we really need that API. It might be worth a quick experiment though
  • I do think maybe _CodemodWrapper is a better name and it would be a good dataclass
  • The main difference between _CodemodContainer/Wrapper and BaseCodemod is that the container knows about the registry and associated metadata and the base codemod does not. But maybe there's a good way to consolidate those two things somehow. I think that's a future effort though.

Copy link
Contributor

@clavedeluna clavedeluna left a comment

Choose a reason for hiding this comment

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

I'm liking this a ton., Just left some thoughts around naming to clarify concepts.

src/codemodder/__main__.py Outdated Show resolved Hide resolved
codemods: list


class _CodemodContainer:
Copy link
Contributor

Choose a reason for hiding this comment

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

Both CodemodCollection and CodemodContainer sound to me like containers of some sort, such as a list. What about making one of these inherit from list? Maybe that's a later thing, just a thought.

Also, how about renaming _CodemodContainer to CodemodMetadata or CodemodWrapper? Also I think this class in particular could be a dataclass. What clues me in to that is all the property methods, but some will have to be manual, of course.

codemods: list


class _CodemodContainer:
Copy link
Contributor

Choose a reason for hiding this comment

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

More thoughts, since you have the idea of this class _CodemodContainer being the "real" BaseCodemod, what do you think is the distinction between the data a BaseCodemod class should store versus this new _CodemodContainer? I still feel they should be one class but I'm listening!

@drdavella drdavella merged commit ca2b7f5 into main Sep 27, 2023
6 checks passed
@drdavella drdavella deleted the codemod-registry branch September 27, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants