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

Remove Generic Handler Support / Implementation (Leaving Failing Tests) #1068

Merged
merged 16 commits into from
Sep 11, 2024

Conversation

zachpainter77
Copy link
Contributor

This PR removes the Generic Handler Support / Implementation and leaves the failing tests for future development / support behaviors as requested from @jbogard

I can't say that I agree with this approach. I feel like it would be better to leave the current implementation in while a better implementation is developed. It doesn't really make sense to add the feature then remove it just to re-add it again in the future when someone can develop it in the way you are wanting. I feel like it's better to just leave the working implementation in the opt in paradigm (opt in feature flag sort of behavior) and let people try to develop in a better way while the current feature is working. That is more of a "Red, Green, Refactor" approach. It doesn't really make sense to me to remove a feature that :

  1. isn't hurting anything (since it is now opt in)
  2. is currently meeting the test criteria that you want the feature to meet.

Those are my thoughts and opinions on it. However, this isn't my choice to make and that is fine.

Cheers.

@jbogard
Copy link
Owner

jbogard commented Sep 9, 2024

Oh I can't merge this without replacing the new behavior, I was going to use this as a starting point for refactoring.

@zachpainter77
Copy link
Contributor Author

zachpainter77 commented Sep 10, 2024

Yeah I think you could just use the opt-in version as a starting point for the refactoring in my opinion. Or we can rename this PR in some sort of way to let people try and get the tests passing with an implementation that satisfies your requirements. Idk.. lol

@jbogard jbogard changed the base branch from master to dynamic-generic-stuffing September 11, 2024 18:38
@jbogard jbogard merged commit cf5bdf5 into jbogard:dynamic-generic-stuffing Sep 11, 2024
3 of 4 checks passed
@jbogard
Copy link
Owner

jbogard commented Sep 11, 2024

Well I don't think I can do this with the stock container. Lamar has support for this kind of thing but it replaces the stock container to do its thing. I'll just keep this existing behavior, even if I don't super like it lol.

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