rss-bridge icon indicating copy to clipboard operation
rss-bridge copied to clipboard

Use "industry standard" coding style (PSR-1, PSR-12) instead of own guide

Open f0086 opened this issue 3 years ago • 9 comments

The current coding style guide is somewhat outdated. A lot of open pull requests struggle with the phpcs settings and need refinement. array() instead of [] is a relic from the past. Same for tabs and some others.

Using PRS-1 + PSR-12 would make the overall experience of contributors much more pleasant, because the IDEs are using it as the default, and most PHP projects are using it. Compatibility with PHP 5.6 are not needed any more.

I only see one problem for changing the coding style guides to PSR-1/PSR-12: The whole codebase need to be migrated in one big commit, which is not a big problem, BUT the 103 open pull requests will all fail to merge or need adjustments eventually. So before migrating the codebase, the majority of PRs need to be merged or closed to lower the amount of work on that PRs.

What do you think?

f0086 avatar Dec 19 '21 22:12 f0086

Don't transform the new codebase but conform new/changed code to modern style. Because we want to keep the vcs history.

dvikan avatar Dec 19 '21 22:12 dvikan

@dvikan Can you elaborate a little on that "keep vcs history" statement? The history is always there, only a git blame will look a little weird. But that's ok IMHO, you can always watch the history for a single file if you really need to trace the author of a bug.

f0086 avatar Dec 20 '21 19:12 f0086

@dvikan Can you elaborate a little on that "keep vcs history" statement?

When I do git blame I don't want to see "Conform to PSR2". It creates unnecessary friction.

dvikan avatar Dec 21 '21 10:12 dvikan

I disagree with that statement, but I am fine with your suggestion (conform new/changed code to modern style).

f0086 avatar Dec 21 '21 16:12 f0086

What do you think about introducing a new src folder, into which the old code is then gradually moved. In this folder then PSR-12 apply. At the same time one could introduce namespaces in it to follow PSR-4.

Examples

Current path New path New Namespace or class
./lib/ ./src/ (Must be determined in more detail) RssBridge\
./actions/ ./src/Action/ RssBridge\Action\
./actions/DetectAction.php ./src/Action/DetectAction.php RssBridge\Action\DetectAction
./bridges/ ./src/Bridge/ RssBridge\Bridge\
./bridges/YoutubeBridge.php ./src/Bridge/Youtube/YoutubeBridge.php RssBridge\Bridge\Youtube\YoutubeBridge
./formats/ ./src/Format/ RssBridge\Format\
./formats/JsonFormat.php ./src/Format/JsonFormat.php RssBridge\Format\JsonFormat

Art4 avatar Dec 23 '21 08:12 Art4

I agree that the sourcecode needs a structural overhaul, but I am not sure if this can be done in multiple stages. It brings in too much complexity to merge both codebases. To introduce namespaces in one go, another "big commit" is needed, which scares a lot of people here. So in order to move forward, we should start by removing the php 5.6 linter. This lowers the burden for almost all PRs and makes PRs process faster.

f0086 avatar Dec 23 '21 12:12 f0086

I have worked on several projects where the conversion to namespaces happened in several steps. It is definitely possible to split them into many smaller steps. Twig is a very prominent example where this transition has taken place in several steps. I'm applying the same principle to SimplePie right now. I would be happy if I can help in this project as well.

The procedure may look like this.

  1. If possible, add aliases for namespaced classes in old files, create files for pseudo classes in new namespace.
  2. use namespaced classenames in old files.
  3. move code from old files into new namespaced files, move the class_alias to new class, keep pseudo class in old code
  4. Take as much steps as needed for Step 3, until all code is moved
  5. remove old files with pseudo classes, remove aliases in new files.

Not all code from ./lib have to be moved into namespaced classes. Something like lib/rss-bridge.php could move to bootstrap.php or similar. Using PSR-4 will give us the ability to have RSS-Bridge installed as a library through Composer and more easily used in other projects.

Art4 avatar Dec 23 '21 13:12 Art4

By the way, GitHub now also supports .git-blame-ignore-revs file so formatting should not be such an issue for git blame any more: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

jtojnar avatar May 12 '22 22:05 jtojnar

We are down to a few PRs, so the issue with open prs should be minimal, as those few can be rebased.

I am in favor of switching to a formatting standard. Dvikan already ran an auto convert once, so I think we should do this before we create the next release

Bockiii avatar May 14 '22 11:05 Bockiii

we are using a variant of PSR12. see 4f75591060d95208a301bc6bf460d875631b29cc et al

dvikan avatar Jul 06 '23 19:07 dvikan