Skip to content

Add conventions around PHP code in the project #68

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

Merged
merged 3 commits into from
Jun 20, 2022

Conversation

yann-eugone
Copy link
Contributor

@yann-eugone yann-eugone commented Jun 18, 2022

The addition of test agains documentation gave me more ideas about things I could check with tests

  • Classes docblocks: every class / interface must have minimal description.
  • Public methods dockblocks: every public method must have minimal description.
  • Package dependencies usage and declaration: every package must declare explicitely the dependency they are using.
  • Source dependencies synchronisation: the main repository must be synchronized with all packages.

@codecov
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #68 (f85bc1e) into 0.x (f84309f) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##                 0.x       #68   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       781       781           
===========================================
  Files            132       132           
  Lines           2005      2004    -1     
===========================================
- Hits            2005      2004    -1     
Impacted Files Coverage Δ
src/batch-box-spout/src/Reader/HeaderStrategy.php 100.00% <ø> (ø)
...batch-box-spout/src/Reader/Options/SheetFilter.php 100.00% <ø> (ø)
...rc/batch-box-spout/src/Writer/WriteToSheetItem.php 100.00% <ø> (ø)
...trine-dbal/src/DoctrineDBALJobExecutionStorage.php 100.00% <ø> (ø)
src/batch-symfony-console/src/CommandRunner.php 100.00% <ø> (ø)
...atch-symfony-console/src/RunCommandJobLauncher.php 100.00% <ø> (ø)
src/batch-symfony-console/src/RunJobCommand.php 100.00% <ø> (ø)
...njection/CompilerPass/RegisterJobsCompilerPass.php 100.00% <ø> (ø)
...ramework/src/DependencyInjection/Configuration.php 100.00% <ø> (ø)
...rk/src/DependencyInjection/YokaiBatchExtension.php 100.00% <ø> (ø)
... and 32 more

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 f84309f...f85bc1e. Read the comment docs.

@yann-eugone yann-eugone requested review from halundraN and J-Ben87 June 18, 2022 08:44
Copy link
Contributor

@halundraN halundraN left a comment

Choose a reason for hiding this comment

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

This is an old debate and it may not be necessary to reopen it here.

From my point of view, making comments systematic reminds me of what we did some years ago. If it brings a lot in some cases, we also inevitably end up with comments that paraphrase the signature of the methods and make the reading of the code more cumbersome.

Example :

  • "Run a command asynchronously" on CommandRunner::runAsync()
  • "yokai/batch Symfony Bundle" on YokaiBatchBundle
  • "Get a parameter value." on JobExecution::getParameter()

It's good to have comments to explain the code but I have doubts about the facts of making them systematic.

It's just an opinion ;)

@yann-eugone
Copy link
Contributor Author

Yeah you're right @halundraN !
My point is : most of the time, a comment is very welcome, but in some very rare spots it is 100% useless.
This is why I rather want this kind of test, because I'm able to set my own rules to decide whether a comment is required here.
I've already removed non public methods, accessors, ...
But I was not able to add the exceptions for the places you spotted here.

So again, yeah this might be boring at some very particular classes/methods, but most of the time it is good to add.

@yann-eugone yann-eugone merged commit 27cf73a into 0.x Jun 20, 2022
@yann-eugone yann-eugone deleted the test-php-conventions branch June 11, 2023 11:05
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.

3 participants