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

Extract abstract base class BaseInterfaceProxyGenerator #550

Merged

Conversation

stakx
Copy link
Member

@stakx stakx commented Jan 4, 2021

This is similar to my previous PR #549 which extracted a base class for class proxy generators. Only this time, we start off with extracting the base class, then InterfaceProxyWithTargetGenerator specifics are pushed into the subtype.

Again, the commits represent single edit steps (for easier reviewing), they can be squashed when merging.

@stakx stakx force-pushed the dp/refactor/base-interface-proxy-generator branch from 06b1b55 to f8972d6 Compare January 4, 2021 12:24
@stakx stakx force-pushed the dp/refactor/base-interface-proxy-generator branch from f8972d6 to 7e19df6 Compare January 4, 2021 12:26
@stakx stakx merged commit 620de74 into castleproject:master Jan 4, 2021
@stakx stakx deleted the dp/refactor/base-interface-proxy-generator branch January 4, 2021 12:45
Copy link
Member

@jonorossi jonorossi left a comment

Choose a reason for hiding this comment

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

Legend 🥇 . That mess has annoyed me for years, but I've never attempted to clean it up because it always seemed like there were so many small differences between each generator.

Thanks for making it so easy to code review.

@stakx
Copy link
Member Author

stakx commented Jan 19, 2021

Same general feeling here. And there are probably similar opportunities remaining, in fact the two new base proxy generator classes share quite a few bits in common. It seems entirely possible to eventually merge them right into BaseProxyGenerator. My only worry is that the base class may become way too large (but that may be only an intermediate state), it would probably have to be restructured in some way.

@jonorossi
Copy link
Member

Probably need more composition and less inheritance. The changes you made to separate one contributor into 3 for their specific tasks is probably more the direction things should go.

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.

2 participants