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

Declare strict types #2365

Closed
wants to merge 11 commits into from

Conversation

floverdevel
Copy link

@floverdevel floverdevel commented Jan 11, 2019

Hello !
Since #1771 seems not to be be going forward, i forked it, rebased on master and added a commit to remove the public members of the class that were mentioned by @gsherwood here : #1771 (comment)

@michalbundyra
Copy link
Contributor

Hi, I do not understand why we can't have them public? It is configurable, as it was requested before... see: #1771 (comment)

@floverdevel
Copy link
Author

Hi, I do not understand why we can't have them public? It is configurable, as it was requested before... see: #1771 (comment)

see this comment : #1771 (comment)

@michalbundyra
Copy link
Contributor

@floverdevel For me it is unclear. I don't know if @gsherwood changed his mind and he doesn't want to have this sniff configurable anymore?

@gsherwood
Copy link
Member

I don't know if @gsherwood changed his mind and he doesn't want to have this sniff configurable anymore?

I commented on the other PR that I wanted changes to how the configuration works because the proposed public properties (specifically format) are not how other sniffs configure their settings. @floverdevel asked why I don't just merge the PR as-is and change the public properties later and I said that once I release them, that's the API for the sniff and I can't change those public properties until a major release.

@michalbundyra
Copy link
Contributor

@gsherwood ok, so basically you want make $format protected and two other properties keep as public, or you don't like them as well to be configurable? I think these two are consistent with other sniffs.

@gsherwood
Copy link
Member

ok, so basically you want make $format protected and two other properties keep as public

That's not what I said at all. Please read my comment here: #1771 (comment)

@michalbundyra
Copy link
Contributor

@gsherwood sorry, I am so lost... I just understand you don't like $format, and I can understand it. If you don't want any configuration right now, it's fine, we can always add them later if requested, that's right. But I can't see reason to make them protected as these are already implemented and works, I mean:

  • spacesBefore - number of blank lines before the declaration (default 1)
  • spacesAfter - number of lines after the declaration and before the script (default 1)

and I think these are quite useful.

@michalbundyra
Copy link
Contributor

Even more maybe we should be able to allow strict type declaration in the same line as php open tag, I've seen some people/project are doing it, no it's not possible.

@vv12131415
Copy link
Contributor

Is this a thing that really needs to be done?
Why not just use https://github.com/slevomat/coding-standard#slevomatcodingstandardtypehintsdeclarestricttypes-

@gsherwood
Copy link
Member

Is this a thing that really needs to be done?

Yes, it needs to be done for PHPCS to complete its PSR-12 standard.

@gsherwood
Copy link
Member

sorry, I am so lost...

My comments related only to the format property. The format property was being used to decide how many spaces are around the equals sign and parenthesis (for example). I said that it would be better to make these additional properties rather than matching a fixed format property.

I never said that the spacingBefore of spacingAfter properties should be removed. I have no idea why you think that. I specifically asked for them to be renamed and you did that.

@gsherwood gsherwood added this to the 3.5.0 milestone Jan 24, 2019
@michalbundyra
Copy link
Contributor

@gsherwood Ok, all clear now. So I've changed $format visibility to private. Please see original PR #1771. Thanks and sorry for confusion.

sertand added a commit that referenced this pull request Jul 4, 2019
@gsherwood
Copy link
Member

After reviewing this sniff again, the use of the property to check the format of the declare statement made it impossible to properly report and fix errors. Instead, the sniff needed to be rewritten to enforce each of the individual rules of the PSR-12 standard so each message could have a different error code and fix. This also lets it account for PHPCS annotations much more easily.

So I've decided not to merge this PR, but it was very helpful in figuring out what needed to be done. Thank you to those who contributed. I know this took a long time to get done, but hopefully the sniff that goes into 3.5.0 properly enforces declare formatting and has been written to enable customisation in the future.

@gsherwood gsherwood closed this Jul 4, 2019
@gsherwood gsherwood removed this from the 3.5.0 milestone Jul 4, 2019
gsherwood added a commit that referenced this pull request Jul 4, 2019
@jrfnl
Copy link
Contributor

jrfnl commented Jul 12, 2019

@gsherwood I've just had a look at the sniffs which were added to PHPCS 3.5.0, but - in contrast to what was discussed here -, the formatting sniff now merged does not allow for any customization in spaces required on the inside of parenthesis, around the = etc.
Is this intentional ? What happened to everything discussed above ?

@gsherwood
Copy link
Member

Is this intentional ? What happened to everything discussed above ?

Yes, it was an intentional decision to code this for PSR-12's rules only so it could be done in the time allowed.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 16, 2019

@gsherwood Would you be open to a PR at a later time to add the properties as discussed above ?

@gsherwood
Copy link
Member

Would you be open to a PR at a later time to add the properties as discussed above ?

Yes

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.

5 participants