Skip to content

CodeIgniter4 marked as internal #6

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

Closed
tangix opened this issue Oct 20, 2022 · 10 comments · Fixed by #7
Closed

CodeIgniter4 marked as internal #6

tangix opened this issue Oct 20, 2022 · 10 comments · Fixed by #7

Comments

@tangix
Copy link
Contributor

tangix commented Oct 20, 2022

Low-prio issue, but as you have marked CodeIgniter\CodingStandard\CodeIgniter4 as @internal phpstorm complains on me using it. Is there a reason for this class to be internal?

@paulbalandan
Copy link
Collaborator

It is internal in the sense that it is internally used by the framework. But we also allow it to be used by end users. So we can think that it is "semi-internal". If a user will use it, they are not covered by BC promise.

Do we have other options for annotation?

@tangix
Copy link
Contributor Author

tangix commented Oct 20, 2022

Don't know the implications in other places, but the syntax from https://docs.phpdoc.org/2.9/references/phpdoc/tags/internal.html discussing {@internal lorem ipsum} at least silence the warnings in phpstorm and at the same time shows the comment as being recognized (i.e. bold in the editor).
As said, this is a very minor issue ;-)

@paulbalandan
Copy link
Collaborator

Can you try editing your vendor copy and adding which of these lines will phpstorm not complain:

  1. @internal Use of this class is not covered by the backward compatibility promise for CodeIgniter4.
  2. {@internal Use of this class is not covered by the backward compatibility promise for CodeIgniter4.}

@tangix
Copy link
Contributor Author

tangix commented Oct 20, 2022

Just putting anything after @internal seems to do the trick, so both lines do work in phpstorm 2022.3 and removes this warning:
image

@paulbalandan
Copy link
Collaborator

Fantastic! Can you send a PR changing the docblock? Thanks.

@MGatner
Copy link
Member

MGatner commented Oct 20, 2022

I'm surprised by that description in the docs. Static analysis treats @internal as "cannot be used outside this package" which, I believe if no @package is specified, uses the root namespace for determination.

The second option (in {}) seems like a totally different use:

It may also be used inside a long description to insert a piece of text that is only applicable for the developers of this software.

And what we actually want here. If both work with comment text after I would recommend using the brackets to be consistent with their docs.

@tangix
Copy link
Contributor Author

tangix commented Oct 20, 2022

@paulbalandan
Don't know how you handle CHANGELOG so I went with the de-facto standard of adding [Unreleased.

@MGatner
Yeah, the difference between the two syntaxes is interesting.

@tangix
Copy link
Contributor Author

tangix commented Oct 20, 2022

Also - thanks for the work putting this package together. Really nice to keep my own code as clean and well-formatted as CI's. Most of the rules are the same as PSR12 in PHPStorm, I only needed a few tweaks to make them the exactly the same so I can use PHPstorm's built in style formatter and then this as part of the git commit.
Only thing that bit me were the strict rules breaking my code. The override was easy to implement:

    'customRules' => [
        NoCodeSeparatorCommentFixer::name() => true,
        'strict_comparison' => false,
        'strict_param' => false
    ],

(And it was Postman tests that discovered the broken code - lesson learned!)

@paulbalandan
Copy link
Collaborator

overrides should be done in the php-cs-fixer.dist.php, in the $overrides array. customRules should be only for instantiating custom rules

@tangix
Copy link
Contributor Author

tangix commented Oct 20, 2022

overrides should be done in the php-cs-fixer.dist.php, in the $overrides array. customRules should be only for instantiating custom rules

True. I started with the distributed config from the CI4 library and tweaked the ruleset just to get it running.

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 a pull request may close this issue.

3 participants