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

feat: Rename CurlyBraceTransformer to BraceTransformer #7333

Merged
merged 2 commits into from Sep 29, 2023

Conversation

localheinz
Copy link
Member

This pull request

  • renames the (internal) CurlyBraceTransformer to BraceTransformer

Related to #5915 (comment).
Follows #7328 (comment).

πŸ’β€β™‚οΈ Looks like it's not used anywhere, maybe we should remove or use it where it makes sense?

@Wirone
Copy link
Member

Wirone commented Sep 27, 2023

Looks like it's not used anywhere, maybe we should remove or use it where it makes sense?

All transformers are magically registered in PhpCsFixer\Tokenizer\Transformers::registerBuiltInTransformers() and used effectively in PhpCsFixer\Tokenizer\Tokens::applyTransformers() during tokenisation, so we can't remove it πŸ˜‰.

PS. I would wait with such refactorings until we set up final vision in #5915 (especially here).

@coveralls
Copy link

coveralls commented Sep 27, 2023

Coverage Status

coverage: 94.661%. remained the same when pulling 85f8672 on localheinz:fix/curly-brace-transformer into f1abe95 on PHP-CS-Fixer:master.

@localheinz
Copy link
Member Author

@Wirone

Not entirely sure about this change, maybe we would need to rename

final class ArrayTypehintTransformer extends AbstractTransformer

as well (see #7338)?

The transformers refer to PHP tokens, and the constants sometimes use inconsistent naming, too.

@Wirone Wirone changed the title fix: Rename CurlyBraceTransformer to BraceTransformer feat: Rename CurlyBraceTransformer to BraceTransformer Sep 27, 2023
@Wirone
Copy link
Member

Wirone commented Sep 27, 2023

The transformers refer to PHP tokens, and the constants sometimes use inconsistent naming, too.

@localheinz I've already warned you about that in your first PR that touched transformers πŸ˜‰. Some fixers can be renamed safely IMHO, but we really need some decisions in #5915.

@keradus
Copy link
Member

keradus commented Sep 29, 2023

If I miss sth please describe.
As long as we rename internal class, but not exposed CT:: values, we are good to rename things.

If we would rename CT::*, we can break BC.

(and agree that removing it would break our code)

@keradus keradus enabled auto-merge (squash) September 29, 2023 13:53
@keradus keradus merged commit bad5c5f into PHP-CS-Fixer:master Sep 29, 2023
15 checks passed
@localheinz localheinz deleted the fix/curly-brace-transformer branch September 29, 2023 14:17
@localheinz
Copy link
Member Author

Thank you, @keradus and @Wirone!

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.

None yet

4 participants