arkaitzgarro / elastic-apm-laravel

Laravel APM agent for Elastic v2 intake API
MIT License
79 stars 17 forks source link

chore: fix variable casing #84

Closed arkaitzgarro closed 4 years ago

arkaitzgarro commented 4 years ago

We use snake_case to define our variables, properties and function arguments. Fix the variables that don't follow this pattern.

arkaitzgarro commented 4 years ago

To be fair our own README says that you should use PSR-2 and Symfony standards, so not snake case. I don't get why cs fixer is not complaining. Or am I misremembering psr-2...

Oh well, it's a deliberate decision in PSR-1:

This guide intentionally avoids any recommendation regarding the use of $StudlyCaps, $camelCase, or $under_score property names.

I cannot find a rule in php cs fixer to enforce camelCase, how do they do it for Symfony?

countless-integers commented 4 years ago

I cannot find a rule in php cs fixer to enforce camelCase, how do they do it for Symfony?

they do camel case for variables, function, methods and arguments, but snake case for configuration arrays (or array keys in general) and templates (for some reason).

I like that, but tbh. we could also go with snake case and just mention it in the readme and maybe update the cs fixer rules.

arkaitzgarro commented 4 years ago

I cannot find a rule in php cs fixer to enforce camelCase, how do they do it for Symfony?

they do camel case for variables, function, methods and arguments, but snake case for configuration arrays (or array keys in general) and templates (for some reason).

My question was more about how do they enforce these rules. If I open a PR in Symfony repo with the wrong case, will automated tools complain about it, or will it be a human instead?

countless-integers commented 4 years ago

no idea, I'd bet on a bot though. Fabien wrote php-cs-fixer and it contains all symfony cs rules.

dstepe commented 4 years ago

What is your position on applying the snake_case variable name convention to the unit test classes? I notice it is not currently applied. I've been using camelCase for consistency (well, mostly out of habit, but it's happy coincidence).

arkaitzgarro commented 4 years ago

What is your position on applying the snake_case variable name convention to the unit test classes? I notice it is not currently applied. I've been using camelCase for consistency (well, mostly out of habit, but it's happy coincidence).

We might need to revisit our position about snake_case vs camelCase. As @countless-integers mentioned, our own README says that you should use PSR-2 and Symfony standards, so not snake case. Based on that, we should use camelCase for our variable names, but the problem that I personally have is that I want a tool (PHP CS fixer or any other) to do the job for us, I don't want to spend time on "please use X case".

@dstepe Do you know a way to enforce that rule somehow?

countless-integers commented 4 years ago

... I want a tool (PHP CS fixer or any other) to do the job for us, I don't want to spend time on "please use X case".

php-cs-fixer, example config from symfony repo

arkaitzgarro commented 4 years ago

... I want a tool (PHP CS fixer or any other) to do the job for us, I don't want to spend time on "please use X case".

php-cs-fixer, example config from symfony repo

Unfortunately, it does nothing about variable cases 🤷🏻‍♂️

dstepe commented 4 years ago

From this old conversation, it looks like fixing variable names is considered too risky:

https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/2362

The argument being that variable names may be influenced by external libraries you are pulling in to your code or other factors. And php-cs-fixer is a fixer, not a detector, so there is opposition to adding something it cannot fix.

I have no experience with this package, but there does seem to be a developing alternative:

https://github.com/rectorphp/rector

And it explicitly addresses this issue:

https://github.com/rectorphp/rector/pull/3415

Having both in an automated pipeline might cause fights, and I'm not sure if you want to switch (I'm a fan of php-cs-fixer myself), but maybe rector could be used to as an on-demand manual tool to find and fix violations.

arkaitzgarro commented 4 years ago

I'm going to close this PR and give it a second thought. Since this is a package for Laravel, we could even follow their styles defined in styleci.