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

[VarDumper] Use hyperlinks in CliDescriptor #29613

Merged

Conversation

Projects
None yet
6 participants
@ogizanagi
Copy link
Member

commented Dec 14, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets Part of #29585
License MIT
Doc PR N/A

Leverages #29168 to add the hyperlink directly on the source text instead of printing below:

Before (or with symfony/console < 4.3)

capture d ecran 2018-12-14 a 16 37 18

After

capture d ecran 2018-12-14 a 16 12 13

@nicolas-grekas
Copy link
Member

left a comment

cool :)

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

Problem with this is that link will be now inaccessible for users not using terminal which supports hyperlinks

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

@ostrolucky true. I think that's fine personally: most common terminals handle the links already and windows didn't make it clickable anyway.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

yeah not a big deal in this one case, it's really minor inconvenience

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit e54e219 into symfony:master Dec 20, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 20, 2018

feature #29613 [VarDumper] Use hyperlinks in CliDescriptor (ogizanagi)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[VarDumper] Use hyperlinks in CliDescriptor

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | Part of #29585   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Leverages #29168 to add the hyperlink directly on the source text instead of printing below:

#### Before (or with `symfony/console < 4.3`)

![capture d ecran 2018-12-14 a 16 37 18](https://user-images.githubusercontent.com/2211145/50012891-1e2efe00-ffc0-11e8-9e07-b5358cb057bd.png)

#### After

![capture d ecran 2018-12-14 a 16 12 13](https://user-images.githubusercontent.com/2211145/50012921-25560c00-ffc0-11e8-92cd-d6efd43419f9.png)

Commits
-------

e54e219 [VarDumper] Use hyperlinks in CliDescriptor

@ogizanagi ogizanagi deleted the ogizanagi:var_dumper/cli_descr_file_link branch Dec 20, 2018

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

I wonder how this could be leveraged with awesome console PHPStorm plugin

@ogizanagi

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

Well, using this plugin, there's nothing much to do: both source and file sections will already be interpreted by the plugin as links and allow to jump to the source. But only the full file link would allow to jump to the source & line nb.

Problem is though the hyperlink feature used here breaks the whole PhpStorm terminal emulator :/

Perhaps we could maintain a list of terminals actually supporting this feature to avoid such issues (at least ignore hyperlinks for PhpStorm terminal emulator, i.e: TERMINAL_EMULATOR=JetBrains-JediTerm).

fabpot added a commit that referenced this pull request Dec 23, 2018

minor #29642 [VarDumper] Add descriptors tests (ogizanagi)
This PR was merged into the 4.1 branch.

Discussion
----------

[VarDumper] Add descriptors tests

| Q             | A
| ------------- | ---
| Branch?       | 4.1 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Just adding some tests for these classes & fixing a small issue with the optional `project_dir` entry from `source` context provider.

If merged before #29613, I'll update tests in it (796ca6b) (otherwise could be picked on merge).

Commits
-------

8570b8a [VarDumper] Add descriptors tests

chalasr added a commit that referenced this pull request Dec 29, 2018

bug #29668 [Console][VarDumper] Ignore href for PhpStorm terminal emu…
…lator (ogizanagi)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Console][VarDumper] Ignore href for PhpStorm terminal emulator

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29613 (comment)   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

So, as explained in #29613 (comment), the hyperlink feature currently breaks the PhpStorm terminal (the output stops abruptly, sometimes the whole terminal emulator freezes). Currently, a simple `dump(new \Exception());` would be enough to break it.
Hence I think we should at least ignore hyperlinks for this terminal emulator.

📝 https://youtrack.jetbrains.com/issue/IDEA-204536 feature request has been opened on JetBrains YouTrack.

Commits
-------

0f65a76 [Console][VarDumper] Ignore href for PhpStorm terminal emulator

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.