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

Fixed some code style with php-cs-fixer #194

Closed
wants to merge 1 commit into from

Conversation

mpscholten
Copy link
Contributor

This shouldn't change any logic.

Because of the public-keyword we cannot put this in the master, but if requested I'll try to get this ready for master too.

@terrafrost
Copy link
Member

Per my in-line comment I'm going to close this pull request without merging it. Resubmit it without the public's. That'll make it shorter and easier to review.

Everything else I saw looked okay but, for stuff like this... run it against the PEAR CS and post the result. The new pull request should result in less warnings / errors than the current version. eg. less than this:

#189 (comment)

@terrafrost terrafrost closed this Nov 29, 2013
@terrafrost terrafrost reopened this Nov 29, 2013
@terrafrost
Copy link
Member

Because of the public-keyword we cannot put this in the master, but if requested I'll try to get this ready for master too.

I didn't see that comment in your pull request lol.

Anyway, I think we should get this in master. And I think the public-keyword should be removed. I don't think it really adds anything anyway except for potential conflicts.

All in all, I still think this ought to be closed, but since I closed it last time without reading the comment in your pull request I'll leave it open for the time being...

@bantu
Copy link
Member

bantu commented Nov 29, 2013

Yes, all PHP4-compatible changes should be against master. Please send a new PR against master.

I am not sure I see the point of replacing var with public. This will likely just cause unnecessary merge conflicts without any benefits.

Ideally, use at least one commit per rule you are fixing and also add the rule to the code sniffer ruleset. Maybe the ruleset should be changed to PEAR + ignore first to make this easier.

@terrafrost
Copy link
Member

bantu's pull requests kinda invalidate this one now.

@terrafrost terrafrost closed this Dec 3, 2013
@bantu
Copy link
Member

bantu commented Dec 3, 2013

@mpscholten Feel free to take a shot at the stuff that is left. See #197, #198, #199, #200 for how I did it.

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.

None yet

3 participants