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

[BUGFIX] partially satisfy psalm for tests/Unit/CssInlinerTest.php #801

Merged

Conversation

SignpostMarv
Copy link
Contributor

No description provided.

…ertion, re: MyIntervals#537

This commit resolves the PossiblyNull issue identified in tests/Unit/CssInlinerTest.php
by adding an additional assertion that would force the test to fail
if the `DOMNodeList::item()` resolved to a non-element result (i.e.
null or other DOMNode type)

regarding: MyIntervals#537
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Again, we need separate @param and @psalm-param entries in the DocBlock...

tests/Unit/CssInlinerTest.php Show resolved Hide resolved
This commit resolves a set of InvalidOperand issues by using the
psalm-specific docblock tags `@psalm-return`, `@psalm-var`, and
`@psalm-param`

regarding: MyIntervals#801 (review)
@SignpostMarv SignpostMarv force-pushed the psalm--tests/Unit/CssInlinerTest.php branch from 45faa5f to 062f73b Compare October 7, 2019 18:36
@SignpostMarv
Copy link
Contributor Author

Again, we need separate @param and @psalm-param entries in the DocBlock...

resolved in 062f73b

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

LGTM now.

BTW, we probably don't need the full path of the file in the commit title, as there's almost always only going to be one file with that name. A succincter title helps a reader browsing through a list of commits.

@JakeQZ JakeQZ merged commit a4a725d into MyIntervals:master Oct 7, 2019
@oliverklee
Copy link
Contributor

BTW, we probably don't need the full path of the file in the commit title, as there's almost always only going to be one file with that name. A succincter title helps a reader browsing through a list of commits.

For example: [CLEANUP] Fix psalm warnings in CssInlinerTest

@JakeQZ
Copy link
Contributor

JakeQZ commented Oct 7, 2019

BTW, we probably don't need the full path of the file in the commit title, as there's almost always only going to be one file with that name. A succincter title helps a reader browsing through a list of commits.

For example: [CLEANUP] Fix psalm warnings in CssInlinerTest

I amended the title a bit, though not quite like that. Hope what I did was OK...

@JakeQZ
Copy link
Contributor

JakeQZ commented Oct 7, 2019

I amended the title a bit, though not quite like that. Hope what I did was OK...

Is there any way to edit commit messages/titles once the've been merged to master? There should be but I couldn't find a way...

@oliverklee
Copy link
Contributor

Is there any way to edit commit messages/titles once they've been merged to master?

No (at least not with a protected master branch).

@JakeQZ
Copy link
Contributor

JakeQZ commented Oct 7, 2019

Is there any way to edit commit messages/titles once they've been merged to master?

No (at least not with a protected master branch).

That's what I feared. And why on a couple of these PRs I suggested you do the final squash, merge and commit. Because I've already screwed up the commit title/message on a couple of merges. But given the need to move forward, and the lightning speed at which @SignpostMarv seems to be working, I felt I had to commit changes I was perfectly happy with and with which I could not think of any reason you (@oliverklee) would not be.

GitHub/git is good at making sure everything is peer-reviewed, CI-tested, etc. But the one thing it is strikingly lacking is having the final commit title/message peer-reviewed. Or any ability to correct it later (incorrect code changes can always be sorted with a further commit). We should perhaps consider raising an issue with GitHub itself? (The sqaush-and-merge feature is good and useful, but it does apparently introduce such problems.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants