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

TASK: Fix psalm errors #1937

Merged
merged 2 commits into from Mar 16, 2020
Merged

TASK: Fix psalm errors #1937

merged 2 commits into from Mar 16, 2020

Conversation

albe
Copy link
Member

@albe albe commented Mar 15, 2020

Make psalm happy again

@@ -206,11 +206,12 @@ public function get(string $entryIdentifier)

$statementHandle = $this->databaseHandle->prepare('SELECT "content" FROM "' . $this->cacheTableName . '" WHERE "identifier"=? AND "context"=? AND "cache"=?' . $this->getNotExpiredStatement());
$statementHandle->execute([$entryIdentifier, $this->context(), $this->cacheIdentifier]);
/** @var false|string|null $fetchedColumn */
Copy link
Member Author

Choose a reason for hiding this comment

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

The column content is nullable in the schema, so this can return null.

@albe albe changed the title TASK: Fix psalm error about immutable Now TASK: Fix psalm errors Mar 15, 2020
@albe
Copy link
Member Author

albe commented Mar 15, 2020

Having to use psalm-* annotations is so-so, but after all it provides a semantic benefit. Would prefer to have just @immutable but let's embrace that tool fully where it makes sense (mutability and maybe generic semantics are such cases).

Copy link
Member

@daniellienert daniellienert left a comment

Choose a reason for hiding this comment

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

@albe, thought the benefit of psalm is Having the baseline instead of adding annotations. Why is it needed here?

@albe
Copy link
Member Author

albe commented Mar 15, 2020

Not needed, but I think in this case it is more benefit in adding this one annotation than silencing the error. Psalm can this way detect ill-usage of Now in the places it is used.

We can of course baseline this case instead too.

@daniellienert
Copy link
Member

@albe maybe add that explanation to the commit that introduces it

Psalm can this way detect ill-usage of Now in the places it is used, which is better than just silencing the error with a baseline update.
@albe
Copy link
Member Author

albe commented Mar 15, 2020

Done @daniellienert :)

@daniellienert daniellienert merged commit 0662aa0 into 6.0 Mar 16, 2020
@daniellienert daniellienert deleted the albe-immutable-now branch March 16, 2020 18:45
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

2 participants