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.

Sign up for GitHub

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

Jump to bottom

Add support for config files with .php extension #247

Merged
merged 8 commits into from May 20, 2022
Merged

Add support for config files with .php extension #247

merged 8 commits into from May 20, 2022

Conversation

yguedidi
Copy link
Contributor

Fixes #246

Better reviewed commit by commit

Ciloe
Ciloe reviewed May 18, 2022
View reviewed changes
Copy link
Collaborator

@Ciloe Ciloe left a comment

Choose a reason for hiding this comment

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

Seems good, but check tests ?

@OwlyCode
Copy link
Collaborator

Looks good to me! Can we keep the test on the old convention until we release the BC break to remove it?

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022 •
edited

Codecov Report

Merging #247 (5cd71c7) into main (47e4559) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #247      +/-   ##
============================================
+ Coverage     79.00%   79.02%   +0.01%     
  Complexity      749      749              
============================================
  Files            55       55              
  Lines          2410     2412       +2     
============================================
+ Hits           1904     1906       +2     
  Misses          506      506
Impacted Files Coverage Δ
src/Config/ConfigResolver.php 90.00% <100.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47e4559...5cd71c7. Read the comment docs.

@localheinz localheinz mentioned this pull request May 18, 2022
Merged
1 task
@localheinz
Copy link
Collaborator

@Ciloe @OwlyCode @yguedidi

How do you feel about merging #248 first, then we can rebase this branch and adjust the tests?

@localheinz localheinz changed the title Add support for config files with .php extension Add support for config files with .php extension May 18, 2022
@yguedidi
Copy link
Contributor Author

@localheinz looks like a good plan! I should have added tests for the ConfigResolver in the first place 😅

@localheinz localheinz force-pushed the config-php-extension branch 3 times, most recently from 5cd71c7 to 61349a4 Compare May 19, 2022 10:07
localheinz
localheinz approved these changes May 19, 2022
View reviewed changes
Copy link
Collaborator

@localheinz localheinz left a comment

Choose a reason for hiding this comment

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

👍

@yguedidi
Copy link
Contributor Author

thank you @localheinz for the update!
one additional change that could be done is to reverse .twig_cs.dist.php and .twig_cs, so that local config always win over dist config

@localheinz
Copy link
Collaborator

@yguedidi

Sounds good, let me adjust!

localheinz
localheinz reviewed May 19, 2022
View reviewed changes
README.md
Comment on lines +148 to +151
- `.twig_cs.php`
- `.twig_cs`
- `.twig_cs.dist.php`
- `.twig_cs.dist`
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yguedidi

I have turned this into a list, making the order perhaps a bit more clear. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed better!

@localheinz localheinz self-assigned this May 20, 2022
@localheinz localheinz merged commit 6e4c196 into friendsoftwig:main May 20, 2022
@localheinz
Copy link
Collaborator

Thank you, @Ciloe, @OwlyCode, and @yguedidi!

@localheinz
Copy link
Collaborator

@OwlyCode

Do you want to prepare a release later?

@yguedidi yguedidi deleted the config-php-extension branch May 20, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localheinz localheinz localheinz approved these changes

@Ciloe Ciloe Ciloe approved these changes

@OwlyCode OwlyCode Awaiting requested review from OwlyCode

Assignees

@localheinz localheinz

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support config files with .php extension
5 participants
@yguedidi @OwlyCode @codecov-commenter @localheinz @Ciloe