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

Allow to display links on created files #559

Merged

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented Mar 12, 2020

This PR aims to display terminal links on the created file paths when the framework.ide configuration param (or debug.file_link_format container parameter) is set (only displayed if the terminal support it).

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Hi @l-vo!

Sorry for the slow review. I think this is a really cool idea :). Instead of injecting the FileLinkFormatter and $fileLinkFormat into FileManager, could you create a new service (maybe MakerFileLinkFormatter) that handles this logic? It would also handle all the \033]8;;{$href}\033\\{$text}\033]8;;\033\\ type of stuff. Then we can inject that into FileManager.

Thanks!

@l-vo l-vo force-pushed the add_terminal_links_to_created_files branch from 4def163 to 7ced5ea Compare March 28, 2020 20:38
@l-vo
Copy link
Contributor Author

l-vo commented Mar 28, 2020

Hi @weaverryan

Sorry for the slow review

No problem :)

I created a new MakerFileLinkFormatter class and I fixed nullable typed arguments (?string) since the bundle is compatible with php 7.0 applications.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

VERY close now. Could you also add at least one simple test for MakerFileLinkFormatter - just to make sure we don't have a really ugly error in there.

Thanks - I'm looking forward to this!

src/Util/MakerFileLinkFormatter.php Show resolved Hide resolved
src/Util/MakerFileLinkFormatter.php Show resolved Hide resolved
@l-vo l-vo force-pushed the add_terminal_links_to_created_files branch 5 times, most recently from e36a4c8 to 1991ec7 Compare April 12, 2020 13:34
If debug.file_link_format container parameter or framework.ide is configured and terminal supports links
@l-vo l-vo force-pushed the add_terminal_links_to_created_files branch from 1991ec7 to b1553d8 Compare April 12, 2020 13:45
@l-vo
Copy link
Contributor Author

l-vo commented Apr 12, 2020

Updated 🙂

@weaverryan
Copy link
Member

Well-done! A very nice new feature - thank you @l-vo!

@weaverryan weaverryan merged commit 8f15db5 into symfony:master May 4, 2020
@l-vo l-vo deleted the add_terminal_links_to_created_files branch May 4, 2020 14:15
@ThomasLandauer
Copy link
Contributor

I was just about to suggest such a feature ;-) However, this kinda means that it's not working for me :-( What's the expected behavior? Since you're saying "only displayed if the terminal support it", which link syntax are you using?

What I would have expected is a clickable link in this line after php bin/console make:migration:

Next: Review the new migration "src/Migrations/Version20211030130836.php"

I committed a similar feature to phpstan and Codeception.

@l-vo
Copy link
Contributor Author

l-vo commented Oct 30, 2021

@ThomasLandauer It's because make:migration rely on doctrine:migrations:diff. So to have a clickable link, the feature must be bring to the doctrine command.

@ThomasLandauer
Copy link
Contributor

@l-vo thanks! However, I just found out that the line I'm talking about is generated here in this bundle at https://github.com/symfony/maker-bundle/blob/main/src/Maker/MakeMigration.php#L129

So what does your PR do then? I tried it with bin/console make:controller, but still getting no link.

@l-vo
Copy link
Contributor Author

l-vo commented Oct 30, 2021

No, look here:

$migrationOutput = $commandOutput->fetch();
, it's generated by the doctrine:migrations:diff command. Excepted if your terminal is not compatible, it's working for make:controller.

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

3 participants