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

Applies PSR2 rules to almost everything. #258

Closed
wants to merge 5 commits into from

Conversation

Incognito
Copy link

No description provided.

@GrahamCampbell
Copy link

Tests failing?

@WyriHaximus
Copy link
Member

@GrahamCampbell it's a known issue #253

@Incognito
Copy link
Author

It looks specific to php 5.3.3 and related to installing composer deps. I'm not sure how my changes could do this as I don't modify composer.

@Incognito
Copy link
Author

@simensen Is there something you'd like done before merging this PR?

@@ -37,6 +37,7 @@ public function __construct(
FormatterManager $formatterManager,
$dataProviderName,
$dataSingularName = null,
// FIXME violates PSR2; defaults at end of method
Copy link
Member

Choose a reason for hiding this comment

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

Make $filter and $map both default to null. This should fix PSR-2 complaint.

@simensen
Copy link
Member

@Incognito I'm a little nervous with this not being just PSR-2 fixes. There were a lot of logic changing stuff in here that I'd be afraid might break things under certain edge cases. I appreciate you wanting to clean that stuff up but I think it would be al to easier for me to accept if it was just PSR-2 and then maybe each of those de-complexity things as their own PR that we can review on a case-by-case basis.

I'd hate to make a whole lot more work for you, but would you mind pulling those other changes out so we can just focus on the PSR-2 stuff? I'm probably going to be OK with your other changes, I just want to have a chance to review them independently.

Was your PSR-2 stuff just using beautifier or did you do a bunch of it manually?

@Incognito
Copy link
Author

@simensen That's reasonable. I'll update this PR once I'm back at my computer to just do PSR2 auto, then follow up with smaller ones. Thanks for taking a look at this.

@simensen
Copy link
Member

@Incognito Great! Thanks. :)

@GrahamCampbell
Copy link

How about using https://styleci.io/ to automatically generate a PSR-2 fix pull request?

@simensen
Copy link
Member

@GrahamCampbell Good idea. @Incognito I'm going to try that now. :) Will update later.

@ghost
Copy link

ghost commented Oct 13, 2015

this PR should be rebased on top of the master since StyleCI has been used, so it's likely many changes in this PR no longer apply

@Incognito
Copy link
Author

Closing as PR was deprecated.

@Incognito Incognito closed this Feb 2, 2016
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

4 participants