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: add IncludeFunctionRule in default standard fixer #218

Merged
merged 2 commits into from
May 3, 2024

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Apr 30, 2024

hey,
I know there is https://github.com/VincentLanglet/Twig-CS-Fixer/blob/main/docs/configuration.md
but I see a lot of codebase going to this, which is more correct in twig

wdyt of it being standard?

@VincentLanglet
Copy link
Owner

but I see a lot of codebase going to this, which is more correct in twig

Can you list some example of codebase ?

src/Standard/TwigCsFixer.php Outdated Show resolved Hide resolved
@94noni
Copy link
Contributor Author

94noni commented Apr 30, 2024

Can you list some example of codebase ?

ref https://twitter.com/lyrixx/status/1765694838756807153
ref #140

for the ones i had in my mind


could event be usefull to propose to add such tool in the symfony/symfony codebase, as there is the php cs fixer already ^^

@94noni 94noni changed the title [RFC] feat: add IncludeFunctionRule in default standard fixer [WIP] feat: add IncludeFunctionRule in default standard fixer Apr 30, 2024
@94noni 94noni changed the title [WIP] feat: add IncludeFunctionRule in default standard fixer feat: add IncludeFunctionRule in default standard fixer Apr 30, 2024
Copy link
Owner

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think a little before merging this.

I dunno if I want to provide this by default.
Until the tag is deprecated, this might be considered as very opinionated...

Also, I could introduce another standard for such rules...

@94noni
Copy link
Contributor Author

94noni commented Apr 30, 2024

Until the tag is deprecated

do you think its worth it an issue is opened on the symfony/twig repos for the deprecation?

this might be considered as very opinionated...

indeed it can be seen like this, but its the real usage in fine
also the official twig doc note this usage as a semantically more "correct"
https://twig.symfony.com/doc/3.x/tags/include.html

@VincentLanglet
Copy link
Owner

Until the tag is deprecated

do you think its worth it an issue is opened on the symfony/twig repos for the deprecation?

Sure, see https://twitter.com/fabpot/status/1765719169662648337

@94noni
Copy link
Contributor Author

94noni commented May 2, 2024

Sure, see https://twitter.com/fabpot/status/1765719169662648337

I've read this of course, thus this PR
my point was, do you think we (me/you) should create and issue or propose a PR directly on symfony/twig repos ?

does doing this will make this PR advance and your comment this might be considered as very opinionated... more real ?

thank you

@VincentLanglet
Copy link
Owner

I've read this of course, thus this PR my point was, do you think we (me/you) should create and issue or propose a PR directly on symfony/twig repos ?

Sure, I think it's a good idea to propose a PR on twig.

does doing this will make this PR advance and your comment this might be considered as very opinionated... more real ?

If the deprecation is accepted on twig repository, I'll merge 100% this PR.
But it's not a blocker, I think I'll merge it anyway.

@94noni
Copy link
Contributor Author

94noni commented May 2, 2024

I've just opened an issue twigphp/Twig#4067
just to know the process of code deprecating of twig repo (I am not used to it)

lets see how it goes 👍🏻

also just opened a PR at symfony repo to add this tool if it is worth it

@VincentLanglet VincentLanglet merged commit 66f0c3c into VincentLanglet:main May 3, 2024
16 checks passed
@94noni 94noni deleted the patch-1 branch May 10, 2024 06:43
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

2 participants