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 phpstan level 1 errors #487

Merged
merged 3 commits into from Jan 30, 2020
Merged

Fixed phpstan level 1 errors #487

merged 3 commits into from Jan 30, 2020

Conversation

JeroenVanOort
Copy link
Contributor

This fixes all phpstan level 1 errors and enables checking for them in CI. I'm working on a PR to also fix level 2 errors.

@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #487 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #487   +/-   ##
=========================================
  Coverage     98.68%   98.69%           
  Complexity     1757     1757           
=========================================
  Files            66       66           
  Lines          4271     4279    +8     
=========================================
+ Hits           4215     4223    +8     
  Misses           56       56           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 458bf94...36d00ee. Read the comment docs.

tests/VObject/ITip/BrokerTester.php Outdated Show resolved Hide resolved
@@ -608,6 +614,7 @@ protected function nextYearly()
// If we got a byDay or getMonthDay filter, we must first expand
// further.
if ($this->byDay || $this->byMonthDay) {
$occurrence = -1;
Copy link
Member

Choose a reason for hiding this comment

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

which error is fixed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$occurrence may not be defined by the time it is used on line 654.

lib/Property/ICalendar/DateTime.php Show resolved Hide resolved
lib/Property/ICalendar/DateTime.php Outdated Show resolved Hide resolved
tests/VObject/ComponentTest.php Outdated Show resolved Hide resolved
phpstan.neon Show resolved Hide resolved
@JeroenVanOort
Copy link
Contributor Author

May I ask why this is yet to be merged? As far as I know, all requested changes have been done.

@staabm
Copy link
Member

staabm commented Jan 14, 2020

Please give us a few more days for final reviews. Thx for working on it.

@DeepDiver1975 any feedback left?

@phil-davis
Copy link
Contributor

@DeepDiver1975 this has been rebased just now on top of current master.
Please review again.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM

@phil-davis
Copy link
Contributor

@staabm merge, or wait for @DeepDiver1975 to review again?

@staabm
Copy link
Member

staabm commented Jan 30, 2020

Feel free to merge

@phil-davis phil-davis merged commit db2eee8 into sabre-io:master Jan 30, 2020
@phil-davis
Copy link
Contributor

@JeroenVanOort thanks!
Feel free to improve the phpstan level even more ;)

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