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

PossiblyNullArgument: Adding common problem cases and possible solutions #8135

Merged
merged 4 commits into from Jun 21, 2022

Conversation

ThomasLandauer
Copy link
Contributor

See #8133 (comment)

Don't know if this is the best way to explain this, but it's a start :-)

Is there a better way to add the link to https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-mutation-free ?

I also removed the <?php tag from the code block.

See vimeo#8133 (comment)

Don't know if this is the best way to explain this, but it's a start :-)

Is there a better way to add the link to https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-mutation-free ?

I also removed the `<?php` tag from the code block.
@AndrolGenhald
Copy link
Collaborator

Is there a better way to add the link to https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-mutation-free ?

See eg atomic_types.md, just make sure it's correct relative to the current path (../../annotating_code/supported_annotations.md#psalm-mutation-free I think).

@AndrolGenhald AndrolGenhald added the release:docs The PR will be included in 'Docs' section of the release notes label Jun 21, 2022
@AndrolGenhald
Copy link
Collaborator

Could you mention that this is for PossiblyNullArgument docs in the PR title? PR titles are included in the changelog.

@ThomasLandauer ThomasLandauer changed the title Adding common problem cases and possible solutions PossiblyNullArgument: Adding common problem cases and possible solutions Jun 21, 2022
@AndrolGenhald
Copy link
Collaborator

You missed the second link 🙂

It looks like the failing test was fixed on master, could you rebase to the most recent commit on master?

@ThomasLandauer
Copy link
Contributor Author

Rebase: Um, sorry, I did it on GitHub's website. But I could open a new PR (it's just one file anyway), but how do I start off this most recent commit?

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Jun 21, 2022

Eh, it's probably fine, I bet @orklah will be fine merging this despite the failing test since it's obviously not the cause of it.

I'd have thought GitHub would have a way to rebase a PR through the interface, but it doesn't seem possible afaict.

@orklah
Copy link
Collaborator

orklah commented Jun 21, 2022

The CI failure is indeed caused by this PR.

Psalm automatically checks each code example in documentation of issues to ensure each is indeed emitting the mentionned issue.

The fact that you removed <?php makes the code invalid and Psalm is not able to find the error anymore.

Could you try reverting that change so we can check tests pass again?

@AndrolGenhald
Copy link
Collaborator

The CI failure is indeed caused by this PR.

Oh, I saw the first CI failure from muglug's comma issue and thought that was the only problem 😬

Now I'm wondering why that doesn't show up anymore. I guess tests are automatically run on an already-merged copy? That's actually pretty neat and catches a lot of potential subtle issues!

@orklah
Copy link
Collaborator

orklah commented Jun 21, 2022

Thanks!

@ThomasLandauer
Copy link
Contributor Author

@orklah Just noticing that this is still not online at https://psalm.dev/docs/running_psalm/issues/PossiblyNullArgument/ - when are those pages rebuilt?

@orklah
Copy link
Collaborator

orklah commented Sep 22, 2022

I'm not actually sure... It may be that the pages are still built from 4.X but it seems strange... I'll keep this in my todo list to check when I have some free time

@orklah
Copy link
Collaborator

orklah commented Nov 10, 2022

@ThomasLandauer this was fixed :) Docs were not rebuilt

@ThomasLandauer
Copy link
Contributor Author

Thanks - see #8696 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:docs The PR will be included in 'Docs' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants