Codingstyle – contributor input wanted

[rarila]

I’m a fan of a consistent looking codebase as IMHO makes it easier to read the code.

I think we all know the pros of coding style and also the cons – you need a bit of discipline when writing code. But the good thing is, nowadays we have a lot of good tools (IDEs, phpcs, ...) that makes things a lot of easier.

So I wanna ask you contributors, what do you think?!

One thing that annoys me most in Psalms codebase is the mix of fully qualified names and imports in the sourcecode. I’ve even seen imported stuff being used with fqn in the same file. So that would be the first thing I would start with. This will make a big PR for sure, but in the end the code base will profit from it.

[psalm-github-bot]

Hey @rarila, can you reproduce the issue on https://psalm.dev ?

[rarila]

🔴 I'm sorry psalm-github-bot, I'm afraid I can't do that…

[orklah]

🔴 I'm sorry psalm-github-bot, I'm afraid I can't do that…

😄

One thing that annoys me most in Psalms codebase is the mix of fully qualified names and imports in the sourcecode.

Me too! I talked with @weirdan about that recently, this bugs me as well (especially since they're pretty long and we're limited in line length...)
The only weird thing, is that PHP-Parser have multiple class with the same name in different namespaces. For example the Plus class exist in BinaryOp namespace for + and in AssignOp namespace for += and those two can be used in the same file.

[rarila]

Nothing that can’t be done with a use … as …;.

One con is probably the fact that there are a lot of PRs open but otherwise the rebasing should be fairly simple, as there are mostly no logical changes.

[orklah]

Oh, don't mind that, a lot of those are mine and they'll be simple enough to rebase.

[weirdan]

Myself I grew used to partially qualified names. E.g. Type\Atomic\String looks good to me.

[orklah]

I don't dislike that but I still find this very long. Nowadays, any good IDE will fill you in pretty quickly if you need to know what class you're dealing with exactly, and in the big majority of cases, you already know what you're dealing with and it just clutters the code

[rarila]

@weirdan Hmm, maybe nice in some cases, but I’m with orklah on this.
And another thing. This is probably not easy to enforce system wide and consistent with phpcs. Also it puts the burden on the contributor to decide when to use a partially one and where to break up the namespace. So from the perspective of simplicity I would prefer fully imported namespaces.

Do you think you could live with them?

[weirdan]

Do you think you could live with them?

I've been conditioned to use camel case for more than a decade, and Psalm uses snake case for variables. I can live with anything.

[rarila]

Better a bad standard, than no standard ;-)

Regarding snake case, same with me. I can’t remeber having seen a PHP project using snake case for quite a while. But I must say, it looks nice.

Anyway, changing to camel case would probably a bit toooo much :-)

[weirdan]

Yeah, Matt once considered doing that but then abandoned this idea.

[rarila]

WIP…

first I started out to only use phpcbf thinking with this mass of changes software is more capable to do the job errorfree than a human.

One thing learned sofar. phpcbf isn’t able to do all the work. It seems it isn’t able to handle advanced Psalm stuff in php-doc:

/** @var \SimpleXMLElement $e->ignoreFiles */
=>
/** @var SimpleXMLElement $e ->ignoreFiles */

(note the space), or

/** @return null|'not-callable'|\Psalm\Internal\MethodIdentifier */
=>
/** @return null|not-callable|MethodIdentifier */

Also before touching php-doc 2 of the Psalm test failed. I haven’t found out why. Further investigation is needed. Probably also some Psalm related stuff as all unit tests work as expected.

I'm also thinking about if it would be better to make one really big PR or multiple smaller ones splitted into handling classes/functions/constants/… seperately or spltted by folders (eg. src, tests, … or even smaller parts). What would be better from a merging and reviewing standpoint?

[weirdan]

I'd prefer a single PR.

[rarila]

Ok, found the reasons of failing:

• Two times it was simply a cache problem - deleting Psalms cache solved the issue
• One seems to be a bug in Psalm that needs further investigation. I have marked and supressed it and will file a bug report after this is merged

[rarila]

Done…
I left quite some partials for a later PR as I don’t be sure how to handle them best. Neither option (full import, partials, aliases) seems to be best. One reason that makes me think we need some structuring is the ambiguous naming of some classes (no pre- or postfix) and that Psalm has quite some huge classes that then of course import an immense number of namespaces. One decision helper could also be some tools support.