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

Add ResponseHelperCallToJsonResponseRector #252

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

Stoffo
Copy link
Contributor

@Stoffo Stoffo commented Sep 17, 2024

No description provided.

@Stoffo Stoffo force-pushed the add-json-response-rector branch from 4756e0b to 4806512 Compare September 18, 2024 19:01
@Stoffo Stoffo force-pushed the add-json-response-rector branch from 4806512 to 09651c1 Compare September 18, 2024 21:01
@Stoffo Stoffo requested a review from GeniJaho September 19, 2024 15:20
@peterfox
Copy link
Collaborator

It might be better to scale this up to be a rule to handle all response helper scenarios.

Or to make a new configurable rule for the main rector repo. FuncCallToMethodCallRector kind of does the same thing but it instead injects a property into a class via the constructor. I'd say a MethodCallToNew might make sense.

@GeniJaho
Copy link
Collaborator

GeniJaho commented Oct 1, 2024

It might be better to scale this up to be a rule to handle all response helper scenarios.

I don't think it's possible to add them all under one configurable rule, I suspect other response() methods require more than just passing parameters to the new constructor. We could add them in other rules, and make a set out of it 👀

Or to make a new configurable rule for the main rector repo.

A new MethodCallToNew in the core repo would probably do the trick, but I'm not sure it can be done in a way that handles different use cases. We have a simple one here, just copy the method params into the class constructor, but how useful/common would that Rector be for other use cases? However, this makes me think we need some core helper rules for common transforms like these...

@peterfox so how do you want to handle this PR? 😅

@peterfox
Copy link
Collaborator

peterfox commented Oct 1, 2024

@GeniJaho I think for now it's not a bad rule. Let's get it in and then if we find it doesn't scale we'll deprecate this for something else.

@GeniJaho GeniJaho merged commit 7f8c107 into driftingly:main Oct 1, 2024
5 checks passed
@Stoffo Stoffo deleted the add-json-response-rector branch October 7, 2024 14:24
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.

3 participants