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

feat: setup phpstan & rector #89

Merged
merged 8 commits into from
Oct 26, 2023
Merged

feat: setup phpstan & rector #89

merged 8 commits into from
Oct 26, 2023

Conversation

shavounet
Copy link
Contributor

@shavounet shavounet commented Oct 13, 2023

Why?

$quality += 2

How?

Setup PHPStan & Rector without caring about fixing everything : that will come later

@shavounet shavounet force-pushed the feat/ci-quality branch 3 times, most recently from 498b5be to 53c908c Compare October 13, 2023 09:25
@shavounet shavounet marked this pull request as ready for review October 13, 2023 09:29
@shavounet shavounet requested a review from a team as a code owner October 13, 2023 09:29
Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

🤞

$this->sendErrorEvent($request, $reason);
return $response;
},
function (\Exception $reason) use ($request): never {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know that we could define a return type on a closure... and I'm not sure to understand how it could be a good thing to do so... but I guess we can let rector do whatever it wants 🤷


/** @var string */
protected $clientId;
protected $events = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

Suggested change
protected $events = [];
protected array $events = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's a rector change, I won't touch this (might be a BC break if this property is badly used -- maybe this should be in a new PR)


/**
* Constructor
*
* @param string $clientId
*/
public function __construct(EventDispatcherInterface $eventDispatcher, $clientId)
public function __construct(EventDispatcherInterface $eventDispatcher, protected $clientId)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

Suggested change
public function __construct(EventDispatcherInterface $eventDispatcher, protected $clientId)
public function __construct(protected EventDispatcherInterface $eventDispatcher, protected string $clientId)

Copy link
Contributor Author

@shavounet shavounet Oct 26, 2023

Choose a reason for hiding this comment

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

I've applied the 1st change (a524310) but not the 2nd as it might be a BC break (could be done in a new PR I suppose, but I don't want to risk it now)

# TODO add --prefer-lowest once we're ready to handle it
fail-fast: false
steps:
- uses: actions/checkout@master
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- uses: actions/checkout@master
- uses: actions/checkout@v4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 64753a1

@shavounet shavounet force-pushed the feat/remove-curl-factory branch from 0126cff to 0c247de Compare October 26, 2023 09:24
Base automatically changed from feat/remove-curl-factory to master October 26, 2023 09:25
@shavounet shavounet merged commit c81c5ad into master Oct 26, 2023
16 checks passed
@shavounet shavounet deleted the feat/ci-quality branch October 26, 2023 09:31
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