Skip to content

Conversation

carlos-granados
Copy link
Contributor

Updates our code to use the Symfony coding style

Replaces the default value for 5 rules:

  • concat_space In the Symfony standard all spacing around the concat (.) operator is removed. I believe this just makes the code much less readable
  • phpdoc_separation This is just modified so that it won't insert blank lines between Given, When and Then annotations
  • global_namespace_import Symfony does not use imports for Classes and functions in the global namespace (for example it will use \DateTime instead of adding a use DateTime; statement). I think this goes against what we have been enforcing in our project
  • single_line_throw With the Symfony setting all throw statements use a single line but if we do this we end with some very long lines
  • yoda_style Symfony uses yoda comparisons but in these days of static analysers this is no longer really useful and it just makes the code more unreadable

Please let me know if you disagree with any of these exceptions to the Symfony standard

There are a huge number of files changed. As in previous CS pull requests I have separated the changes in different commits with logically related rules to make this easier to review. Some of these commits are themselves quite big even if they result from the application of a single rule, but in those the changes will be all very similar so I am hoping they will be easy to review. Let me know if you would prefer me to split these large commits into smaller ones.

As in other CS pull requests, once everyone is OK with the changes, I will add another commit with the git blame ignore list of commits that need to be ignored

Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

@carlos-granados this looks generally great to me. A couple of personal opinions on style that I'm happy to be overruled on but other than that completely agree with the rules you've applied.

Only if you were going to make any changes to the rules it might be worth splitting the "remove @inheritdoc / unnecessary PHPdoc params" commit to a separate PR first. That was the one github struggled to render for me, and it seemed like it then upset their logic for identifying which files could safely be marked as "Viewed" when looking at a single commit, which made it a little slower to review the subsequent ones.

@carlos-granados
Copy link
Contributor Author

carlos-granados commented Apr 24, 2025

@acoulton I rebased this code and added some changes

  • I would prefer not to split the changes of the bigger commit into a separate PR as many of these changes are interconnected, what I did is split that commit in two, hopefully that will help a little
  • I understand what you mean about better visibility of the ! operator when surrounded by spaces but since this is not part of any current standard I think it is best if we don't follow that for now
  • I removed the commit that aligned the PHPDoc params and added a new exception to the Symfony standard
  • I don't think that defining the $outputDecorated as bool will be a problem. The setter does not have a strict type but the parameter is clearly tagged as bool so I think it should be safe to asume that no one else will be setting it to anything else

Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

@carlos-granados this is great, thanks. My CPU definitely screamed less at having the phpdoc changes in two commits 😆 thanks for overriding the alignment rule.

Agreed on the ! operator. Also on the outputDecorated: thinking about it we're not calling those setters in strict type mode anyway, and I can't imagine people are calling it with something that can't at least cast to a bool so it should be fine.

@carlos-granados carlos-granados merged commit 2ea762d into Behat:master Apr 25, 2025
18 checks passed
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.

2 participants