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

minor: Transfer HelpCommand::toString() to Utils #7034

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

paulbalandan
Copy link
Contributor

Since HelpCommand is internal, I did not mind deprecating the previous.

@SpacePossum
Copy link
Contributor

I would go the other way around, moving all document-generation related stuff to its own namespace.
This way we prevent the generic Utils in the root namespace to become a big collection of all sorts.
Unless I've missed some talk about this logic move

@Wirone
Copy link
Member

Wirone commented Jun 7, 2023

@SpacePossum I suggested it here because It looks like serious boundary-break when you use CLI command's method inside fixers' logic 😅. Since it's a universal conversion, IMHO it fits well to Utils that already has similar methods (like camelCaseToUnderscore()).

@Wirone Wirone merged commit d86065f into PHP-CS-Fixer:master Jun 7, 2023
14 checks passed
@Wirone
Copy link
Member

Wirone commented Jun 7, 2023

Thank you @paulbalandan 🍻

@paulbalandan paulbalandan deleted the extract-to-string branch June 7, 2023 00:22
@keradus
Copy link
Member

keradus commented Jun 7, 2023

agree that XyzFixer should not call HelpCommand

Also, agee with @SpacePossum that we should not add any logic to generic-purpose Utils [it's like random root-level namespace, one doesn't know where to put sth? they made Utils that is having everything].

I don't understand why suggestion of Space to create dedicated class for document generation was not consider and PR merged without concluding on that idea.
IMHO this PR should be reverted.

@Wirone
Copy link
Member

Wirone commented Jun 7, 2023

Utils was already in the repo, was not deprecated, and already had similar generic-purpose methods, so it looked like fine place to move that logic to get rid of weird coupling between fixers and CLI command. IMHO state of the code after PR is better than it was before, even if it's not perfect.

I don't agree it should be reverted, instead we can make another PR that would deprecate Utils and would introduce better place for methods moved in this PR.

I evaluated @SpacePossum's comment and provided my reasoning for merging as-is. It does not mean we can't change this decision 🤷‍♂️.

@keradus
Copy link
Member

keradus commented Jun 7, 2023

Utils was already in the repo, was not deprecated

not a reason to add any random logic to it and make Utils even bigger and more generic.

@keradus
Copy link
Member

keradus commented Jun 7, 2023

I evaluated @SpacePossum's comment (...)

would be awesome to give Space time to get familiar with your answer and allow for discussion rather than concluding to merge directly afterwards

@Wirone
Copy link
Member

Wirone commented Jun 7, 2023

But seriously, what's the problem here? We have 1 new public method in internal class that is considered as improper place for it, instead of place that was even more improper 🤷‍♂️. Work was done voluntarily, can be refactored at any point. Is it really something that we should discuss that much? There are many open PRs waiting for your response, you could utilise this time better 😉.

@keradus
Copy link
Member

keradus commented Jun 7, 2023

god-object antipattern should not be feed with more spam into it.

so if i get poked in private to check the PR due to the concern, i put effort here and not in place that would take bigger benefit from it.

@keradus
Copy link
Member

keradus commented Jun 7, 2023

#7039

@SpacePossum
Copy link
Contributor

Sorry for starting off this discussion as this wasn't my intent. Still think it would've been better to give a bit more thought about how the composition of the classes would ideally look like in the future. Than reviewing would also be more easy as a PR can been seen if it is part of the "bigger plan" or not. And please be considered when talking about peoples time as we all got to manage that as good as we can somehow. Anyway, for now all good imho 👍 Thanks keradus for 7039 :)

niklam pushed a commit to niklam/PHP-CS-Fixer that referenced this pull request Jun 19, 2023
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.

None yet

4 participants