Skip to content

Conversation

TomasVotruba
Copy link

@TomasVotruba TomasVotruba commented Nov 14, 2017

Why?

This package would be still available for PHP 5.6-.

What do you think?

Copy link
Collaborator

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. I like this.

Did you do these manually or did you just a script/tool? Which one?

Also, check the test code, we cannot simply just remove annotations like that.


/**
* @test
* @expectedException \Github\Exception\MissingArgumentException
Copy link
Collaborator

Choose a reason for hiding this comment

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

These annotations cannot just be removed.

Copy link
Author

@TomasVotruba TomasVotruba Nov 14, 2017

Choose a reason for hiding this comment

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

Sure, I got one broken Sniff that causes this.

Will fix, thannks

@acrobat
Copy link
Collaborator

acrobat commented Nov 14, 2017

Why not bump to minumum php 7.1? 7.0 will "lose" it's active support in 18 days (http://php.net/supported-versions.php)

I will try to do a full review in the next few days!

@TomasVotruba
Copy link
Author

TomasVotruba commented Nov 14, 2017

@Nyholm Thanks, I'll get to that.

It's cherry picked coding standard rules + handful of own sniff/fixers.

@acrobat I completely agree!

}

/*
* Add user to organization
Copy link
Contributor

@GrahamCampbell GrahamCampbell Nov 14, 2017

Choose a reason for hiding this comment

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

As StyleCI points out here, there's a syntax error here.

image

Should be:

    /**
     * Add user to organization.
     */
    public function add($organization, $username)

Instead of

         * Add user to organization
     */
    public function add($organization, $username)

Copy link
Author

@TomasVotruba TomasVotruba Nov 15, 2017

Choose a reason for hiding this comment

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

Also broken sniff/fixer.

Will fix it, thanks

@TomasVotruba
Copy link
Author

Replaced by #651 with passing tests and fixed coding style

@TomasVotruba TomasVotruba deleted the rector-nai branch November 16, 2017 15:05
@TomasVotruba TomasVotruba mentioned this pull request Nov 16, 2017
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.

4 participants