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 patching from Generate commands #364

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Remove patching from Generate commands #364

merged 1 commit into from
Aug 27, 2021

Conversation

simivar
Copy link
Contributor

@simivar simivar commented Apr 30, 2021

As discussed in #354 we can remove "patching" existing files from Generate commands as it is quirky and doesn't provide much value and only complicates the codebase.

@veewee
Copy link
Contributor

veewee commented Apr 30, 2021

Thanks for the pr. Will take a deeper look soon.
This will introduce a bc break. Maybe time to list some things we want to change for v2 @janvernieuwe ?

@simivar
Copy link
Contributor Author

simivar commented May 3, 2021

If I think correctly you mean it's BC break because we've deleted a protected method that could've been extended by someone. If that's the case what do you think about, for now, removing only the body contents so we could get it to 1.x version, marking it as @deprecated, and then removing it altogether in 2.x?

@veewee
Copy link
Contributor

veewee commented May 3, 2021

Yes indeed, but on top of that - it is a BC break for the people who might rely on this feature to work this specific way. After the change, the cli tool will work in a different way. This might result in broken code downstream.

@veewee veewee added this to the 2.0.0 milestone May 12, 2021
@veewee veewee changed the base branch from master to v2.0.0 May 12, 2021 13:59
@veewee
Copy link
Contributor

veewee commented May 12, 2021

@simivar
I've pointed this PR to a new 2.0.0 branch in which we will introduce this change.
We could also accept a PR for v1.0.0 that introduces deprecation warnings. But that logging might get lost in the output of the CLI command - so for me it is also fine to describe it in an upgrading section when releasing v0.2.0

@veewee veewee merged commit 8dc4768 into phpro:v2.0.0 Aug 27, 2021
@veewee
Copy link
Contributor

veewee commented Aug 27, 2021

Sorry for the long delay. Wanted to play around with this one before merging :)

Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants