Skip to content
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

Fix more CS #9303

Merged
merged 7 commits into from
Jan 20, 2024
Merged

Fix more CS #9303

merged 7 commits into from
Jan 20, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 4, 2024

No description provided.

@alecpl
Copy link
Member

alecpl commented Jan 5, 2024

  • I don't like "php_unit_test_case_static_method_calls". I don't see a value.
  • With "php_unit_data_provider_name" change some method names look bad now, e.g. provideParse_attrib_stringCases. Either we change all method names (including test* ones) to camel-case or revert that.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 5, 2024

I don't like "php_unit_test_case_static_method_calls". I don't see a value.

Sooner or later is should be done as static calls should be called statically otherwise phpstan will warn about it. repro: https://phpstan.org/r/3c8d1087-b72c-41be-acf8-a2f72d4d788e

With "php_unit_data_provider_name" change some method names look bad now, e.g. provideParse_attrib_stringCases. Either we change all method names (including test* ones) to camel-case or revert that.

It is managed by CS fixer thus even if the provider methods does not look best now, the CS is enforced and once the test methods are renamed to camel case, they will be updated automatically.

@alecpl
Copy link
Member

alecpl commented Jan 6, 2024

I don't like "php_unit_test_case_static_method_calls". I don't see a value.

Sooner or later is should be done as static calls should be called statically otherwise phpstan will warn about it. repro: https://phpstan.org/r/3c8d1087-b72c-41be-acf8-a2f72d4d788e

Only in "strict rules" mode. So, I'm not convinced.

With "php_unit_data_provider_name" change some method names look bad now, e.g. provideParse_attrib_stringCases. Either we change all method names (including test* ones) to camel-case or revert that.

It is managed by CS fixer thus even if the provider methods does not look best now, the CS is enforced and once the test methods are renamed to camel case, they will be updated automatically.

But I don't want it in this incomplete state.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 6, 2024

I have reworked this PR.

php_unit_test_case_static_method_calls - the projects uses snake_case, thus yes, snake_case is more consistent

php_unit_test_case_static_method_calls - let's land this, we have already a lot of changes by other CS changes, so the best time to land this is now - calling static methods statically is definitely more wanted

@mvorisek
Copy link
Contributor Author

@alecpl can you merge this PR? I would like to fix other CS based on it.

@alecpl
Copy link
Member

alecpl commented Jan 16, 2024

  1. I still don't like "php_unit_test_case_static_method_calls". I don't see a value.
  2. I'm not sure about all these empty lines removed. Definitely sometimes inside a switch they make the code cleaner.

@mvorisek mvorisek changed the title Fix CS (whitespace, phpunit static calls) Fix CS (whitespace) Jan 17, 2024
@mvorisek mvorisek changed the title Fix CS (whitespace) Fix more CS Jan 17, 2024
@mvorisek
Copy link
Contributor Author

I have reverted "php_unit_test_case_static_method_calls" changes. I stand by them, but let's discuss them in a separate PR to unblock this one.

@alecpl alecpl merged commit b1a0067 into roundcube:master Jan 20, 2024
15 checks passed
@mvorisek mvorisek deleted the more_basic_cs branch January 20, 2024 07:32
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