Skip to content

Add links to docs in default results output #2978

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

Closed
Danack opened this issue Mar 16, 2020 · 15 comments
Closed

Add links to docs in default results output #2978

Danack opened this issue Mar 16, 2020 · 15 comments

Comments

@Danack
Copy link

Danack commented Mar 16, 2020

Trying to find the docs for some errors is harder than ideal, as examples are showing up in preference to the docs.

e.g. Searching for php psalm MixedAssignment does not include the Psalm docs in the first results, but does include multiple example snippets like: https://psalm.dev/r/34544e24dd

I needed to explicitly search under the docs directory with "MixedAssignment site:https://psalm.dev/docs/" to be able to find the docs.

Any way to improve the discoverability of the docs would be great.

I guess either by excluding the examples from googles indexing, or possibly chucking them under a different sub-domain (, as it could be possible that google is ranking them so highly as they are on the same domain as the main site) would be possible improvements.

@psalm-github-bot
Copy link

psalm-github-bot bot commented Mar 16, 2020

I found these snippets:

https://psalm.dev/r/34544e24dd
<?php

$rows = (array) $_GET['rows'];
$entities = [];

foreach ($rows as $row) {
  $entities['string'][] = 'string';
}

foreach ($entities['string'] as $string) {
  takesBool($string);
}

function takesBool(bool $b) : void {}
Psalm output (using commit 4232bfb):

INFO: PossiblyUndefinedStringArrayOffset - 3:17 - Possibly undefined array offset 'string(rows)' is risky given expected type 'array-key'. Consider using isset beforehand.

INFO: MixedAssignment - 6:19 - Unable to determine the type that $row is being assigned to

ERROR: PossiblyUndefinedArrayOffset - 10:10 - Possibly undefined array key $entities['string']

ERROR: InvalidScalarArgument - 11:13 - Argument 1 of takesBool expects bool, string(string) provided

INFO: UnusedParam - 14:25 - Param $b is never referenced in this method

INFO: UnusedVariable - 6:19 - Variable $row is never referenced

@weirdan
Copy link
Collaborator

weirdan commented Mar 16, 2020

I think we should provide a link to issue description in the Psalm output (something like https://psalm.dev/issue/MixedAssignment). This will both be easier for users (you don't have to google it, just click the link) and will push those pages higher in google rankings because people will post their Psalm output containing the links.

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2020

Yeah – but that could also make the output a little noisy, unless the urls were shortened e.g.

ERROR: DuplicateArrayKey - tests/UnusedVariableTest.php:1028:13 - Key 'usedAsMethodName' already exists on array (see https://psm.dev/14a7)

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2020

Sorbet does this: https://sorbet.run

@muglug muglug changed the title Finding docs hard due to examples showing up. Add links to docs in default results output Mar 16, 2020
@weirdan
Copy link
Collaborator

weirdan commented Mar 16, 2020

I would try to truncate non-essential parts of the URL, but keep the domain and issue name fully spelled out:

ERROR: DuplicateArrayKey - tests/UnusedVariableTest.php:1028:13 - Key 'usedAsMethodName' already exists on array (see https://psm.dev/14a7)

c.f.

ERROR: DuplicateArrayKey - tests/UnusedVariableTest.php:1028:13 - Key 'usedAsMethodName' already exists on array (https://psalm.dev/i/DuplicateArrayKey)

Scheme, however, may need to be kept to make URLs clickable in the terminal output.

@Danack
Copy link
Author

Danack commented Mar 16, 2020

If you use a shorter version of the URL, my understanding is that you should definitely set the canonical link in the page with:

<link rel="canonical" "https://psalm.dev/issue/MixedAssignment" />

as otherwise you might 'dilute' the page ranking.

@Danack
Copy link
Author

Danack commented Mar 16, 2020

btw one other improvement that could be made (and I realise this is significant chunk of work, and me volunteering other people to do a lot of work is annoying), is showing what the fix would be in the example for each issue type.

For example, the current doc for the MixedAssignment error is:

MixedAssignment
Emitted when assigning an unannotated variable to a value for which Psalm cannot infer a type more specific than mixed.

$a = $_GET['foo'];

I'm a bit of a newb when it comes to Psalm. I'm sure it'll get easier for me to understand how to apply types to fix these errors, but right now my confidence in me being able to fix this error in less than five attempts is pretty low.

If as well as the code showing the example error, there was example code with the appropriate type info added, I think this would make it easier to go from an error to fixing the issue for psalm users.

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2020

If I created https://psm.dev/[short code] it would just be a 301 redirect to https://psalm.dev/docs/running_psalm/issues/#mixedassignment, so I'm not worried about SEO issues

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2020

one other improvement that could be made (and I realise this is significant chunk of work, and me volunteering other people to do a lot of work is annoying), is showing what the fix would be in the example for each issue type.

I've just added this: f22f5e3

For more in the same vein you can edit this: https://github.com/vimeo/psalm/edit/master/docs/running_psalm/issues.md

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2020

But to the broader point, do you think each issue deserves its own page?

@weirdan
Copy link
Collaborator

weirdan commented Mar 17, 2020

I'm leaning on the side of 'yes'.

@muglug
Copy link
Collaborator

muglug commented Mar 19, 2020

@Danack
Copy link
Author

Danack commented Mar 19, 2020

Lovely.

One thing that might help ux a little is either a link back to the list of issue types, or if it's possible the list on that page also.

@muglug
Copy link
Collaborator

muglug commented Mar 19, 2020

Yeah – adding a breadcrumb would be cool. In the meantime, see https://psalm.dev/032

@muglug muglug closed this as completed in f532dc3 Mar 19, 2020
@Danack
Copy link
Author

Danack commented Mar 19, 2020

Thank you.

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

No branches or pull requests

3 participants