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

ConsoleReport: Add hyperlinks to open file in editor #6850

Merged
merged 4 commits into from
Nov 14, 2021

Conversation

gharlan
Copy link
Contributor

@gharlan gharlan commented Nov 7, 2021

ConsoleReport adds hyperlinks to the file paths to open them in editor. It uses the file format from xdebug.file_link_format (you do not need xdebug extension for this) and falls back to generic file://... link.

Terminals not supporting hyperlinks should ignore these escape sequences, see https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda#backward-compatibility:

Any terminal that correctly implements OSC parsing according to ECMA-48 is guaranteed not to suffer from compatibility issues. That is, even if explicit hyperlinks aren't supported, the target URI is silently ignored and the supposed-to-be-visible text is displayed, without artifacts.

psalm output (hover over one link, you can see the target at the bottom):

Bildschirmfoto 2021-11-07 um 14 36 00

taint analysis:

Bildschirmfoto 2021-11-07 um 14 36 32

This fixes #5798

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Nov 7, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 7, 2021

Are there any provisions for mapped folders? E.g. Psalm running in a docker container sees files at /var/www/ but IDE needs to open them from /home/weirdan/src/project/.

@weirdan weirdan marked this pull request as draft November 7, 2021 16:09
@gharlan
Copy link
Contributor Author

gharlan commented Nov 7, 2021

Are there any provisions for mapped folders? E.g. Psalm running in a docker container sees files at /var/www/ but IDE needs to open them from /home/weirdan/src/project/.

Hmm, do you have an idea how to solve this?

@weirdan
Copy link
Collaborator

weirdan commented Nov 7, 2021

Thinking out loud here.

This can be handled either by Psalm itself emitting host paths or terminal emulator converting paths in links to their host counterparts. The latter is outside of our control and I have doubts of its universal support anytime soon. This leaves us with the need to implement it ourselves.

The mapping is both machine and project specific: Alice may have her projects in /home/alice/src/<ProjectName> while Bob keeps them in C:\Users\Bob\Documents\<CompanyName>\<ProjectName>. Some projects will have their container paths rooted in /var/www/, some may use another prefix like /home/web. This dictates the need for what I would call local project configuration. This deviates from single, committed to git, config file Psalm currently uses. Local mapping specification would have to be .gitignore-d. There are two options currently supported by Psalm:

  • Commit psalm.xml.dist and .gitignore psalm.xml. Developers working on a project would have to copy psalm.xml.dist to psalm.xml and add their path mapping there. Pros: easy to understand, familiar approach used by many tools. Cons: developers would have to update their psalm.xml when upstream psalm.xml.dist changes.
  • Commit psalm.xml that uses XInclude to include file containing path mapping. The <xi:include> would have to have <xi:fallback> element to prevent errors in CI (where local project mapping file is absent). The mapping file would have to be .gitignored. Pros: developers don't need to update their config. Cons: setup is a bit more complicated.

Neither of the options require any additional development as they are already available.

As for the configuration format, I envision something like this:

<psalm>
  <directoryMapping>
     <directory from="<absolute prefix>" to="<absolute prefix"/>
  </directoryMapping>
</psalm>

With that said, it can totally be a separate feature we may add later.

src/Psalm/Report/ConsoleReport.php Outdated Show resolved Hide resolved
src/Psalm/Report/ConsoleReport.php Show resolved Hide resolved
@weirdan weirdan marked this pull request as ready for review November 14, 2021 21:15
@weirdan weirdan merged commit 45e1a1c into vimeo:master Nov 14, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 14, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

Add editor url option
3 participants