Skip to content

style: update coding style with php-cs-fixer 3.46.0#8400

Merged
kenjis merged 17 commits intocodeigniter4:developfrom
kenjis:style-cs-fixer-3.46.0
Jan 7, 2024
Merged

style: update coding style with php-cs-fixer 3.46.0#8400
kenjis merged 17 commits intocodeigniter4:developfrom
kenjis:style-cs-fixer-3.46.0

Conversation

@kenjis
Copy link
Copy Markdown
Member

@kenjis kenjis commented Jan 5, 2024

Description

  • update coding style

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

try {
// ...
} catch (\Exception $e) {
} catch (Exception $e) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is it better to set leading_backslash_in_global_namespace to true for User Guide?
See https://cs.symfony.com/doc/rules/import/fully_qualified_strict_types.html

If we do so, the leading \ will not be removed, and if it is not in the current sample code it will be added.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

leading backlash has no effect when in global namespace, right? so adding it is superfluous.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It has no effect in PHP syntax, but actually most code will be written in some namespace like App\Controllers,
So when there is no leading backslash, if a user copies and pastes the sample code, the code will not work unless they import the class.

Is it better that sample code should work as it is as possible when a dev copies and pastes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In such cases, it may be better to add namespaces for those files.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO the code copied from the user guide should work out of the box. Less experienced users may not know they have to import the class. If there is a way to automatically add an import to our code examples then that would be fine. Otherwise, I would leave it as it is.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the code copied from the user guide should work out of the box.

I agree with this.

import_symbols option imports symbols when the classname in the code is namespaced (containing \).
But Exception in the above sample is not imported.

@kenjis kenjis force-pushed the style-cs-fixer-3.46.0 branch from 2474290 to eb88fd4 Compare January 6, 2024 00:51
}

$object = new Myclass();
$object = new \Myclass();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Where should we put this Myclass?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Libraries?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks. Done


// Call on an instance method
$user = new User();
$user = new \User();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Where should we put thisUser?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Libraries?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

final class AuthenticationFeatureTest extends CIUnitTestCase
{
use AuthTrait;
use \AuthTrait;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Where should we put this trait?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Traits?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@kenjis kenjis force-pushed the style-cs-fixer-3.46.0 branch from b0451da to abb734c Compare January 7, 2024 01:15
@kenjis kenjis force-pushed the style-cs-fixer-3.46.0 branch from c972f1e to 73cb246 Compare January 7, 2024 01:49
@kenjis kenjis merged commit c0d134a into codeigniter4:develop Jan 7, 2024
@kenjis kenjis deleted the style-cs-fixer-3.46.0 branch January 7, 2024 11:07
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