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

Fix CS config by default too #5467

Closed
wants to merge 6 commits into from
Closed

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 26, 2021

fabbot.io Use ::class whenever possible should be ignored, otherwise PHPStan is not happy, unrelated with this PR...

@coveralls
Copy link

coveralls commented Jan 26, 2021

Coverage Status

Coverage increased (+0.004%) to 91.855% when pulling cc3312c on mvorisek:patch-2 into 9bf2760 on FriendsOfPHP:2.18.

@mvorisek mvorisek force-pushed the patch-2 branch 8 times, most recently from d1d7df5 to 761aed9 Compare January 26, 2021 12:25
src/Finder.php Outdated Show resolved Hide resolved
src/Finder.php Outdated Show resolved Hide resolved
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 4, 2021

@keradus can this be merged and which branch to target?

@kubawerlos
Copy link
Contributor

@mvorisek what about this comment: #5467 (comment) ?

@mvorisek mvorisek force-pushed the patch-2 branch 2 times, most recently from 899d961 to ae64592 Compare April 5, 2021 13:20
@mvorisek mvorisek force-pushed the patch-2 branch 2 times, most recently from 25b8d26 to 929c77c Compare April 5, 2021 13:34
@mvorisek mvorisek force-pushed the patch-2 branch 6 times, most recently from 33f033a to 0140ba9 Compare April 5, 2021 14:19
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 5, 2021

Both comments are now addressed, is everyone happy now? :)

@kubawerlos
Copy link
Contributor

fabbot isn't 😉

@mvorisek mvorisek force-pushed the patch-2 branch 3 times, most recently from 227256b to 90feea6 Compare April 5, 2021 15:06
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2021

fabbot isn't 😉

@kubawerlos I tried to please this bot too with 342d09c, but if I follow the bot advise, I get:

 ------ --------------------------------------------- 
  Line   tests/Console/ConfigurationResolverTest.php  
 ------ --------------------------------------------- 
  208    Class Test1Config not found.                 
  218    Class Test4Config not found.                 
 ------ --------------------------------------------- 

from phpstan. Let me know what to do with this, the fabbot error is unrelated, only some unrelated change was done in that file.

@keradus
Copy link
Member

keradus commented Apr 7, 2021

I'm not convinced for this change, myself. I'm thinking about alternatives

@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2021

I'm not convinced for this change, myself. I'm thinking about alternatives

Please tell your concerns.

@keradus
Copy link
Member

keradus commented Apr 7, 2021

I wasn't concerned. I was not convinced.
For some weeks we were discussing about another naming pattern for config file within core team.
I decided to reject this PR as it's obsolete with new naming pattern: #5607

Thanks for all the effort.

@keradus keradus closed this Apr 7, 2021
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2021

Yes, a lot of work, but new .php suffix makes sense, 👍

@mvorisek mvorisek deleted the patch-2 branch April 7, 2021 17:03
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