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

Feature/ide url handler #265

Merged
merged 13 commits into from Oct 15, 2019
Merged

Conversation

Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Sep 14, 2019

Q A
Bug fix? no
New feature? yes
Fixed tickets #237

Thanks to symfony/console 4.3, we can add hyperlink easily in console.

This PR add a ide config key, (fillable by theses differents editor:
phpstorm, sublime, textmate, macvim, emacs, atom and vscode) and generate hyperlinks in terminal to file that have issues.

@Jibbarth Jibbarth added the enhancement label Sep 14, 2019
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe left a comment

Looks amazing! Just some small comments on it 👏🎉

docs/ide.md Show resolved Hide resolved
docs/ide.md Show resolved Hide resolved
@@ -52,6 +52,10 @@ public function analyse(
$insightCollection = $this->insightCollectionFactory
->get($metrics, $config, $dir, $consoleOutput);

if (method_exists($formatter, 'injectFileLinkFormatter')) {
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe Sep 14, 2019

Choose a reason for hiding this comment

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

Not sure if I like this approach. I feel like it doesn't belong here. Could we just give the format method the config file or the config file in the constructor of the formatter?

Copy link
Collaborator Author

@Jibbarth Jibbarth Sep 15, 2019

Choose a reason for hiding this comment

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

Yeah, I'm not convinced either.

I initially added it in format function, but it's not a prerequisite for each formatter, that's why I think adding it in the constructor it's not a good solution...

Maybe by create an Interface RequireFileLinkFormatter ? And adding it if formatter is instance of this interface.
WDYT @olivernybroe ?

Copy link
Sponsor Collaborator

@olivernybroe olivernybroe Sep 15, 2019

Choose a reason for hiding this comment

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

Hmm, still seems kinda weird as that is only needed for that formatter.

What if we in the constructor of the Console formatter just extract the parameter ide from the inputInteface? This way all the logic is in the Console formatter, but no extra parameter is needed.

Copy link
Collaborator Author

@Jibbarth Jibbarth Sep 15, 2019

Choose a reason for hiding this comment

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

In fact, it's used by the Console formatter, but we can imagine use it also in an HTML Formatter. That's why I would prefer keeping the ide resolving outside the Console Formatter.

Maybe the easier would be to create an Config class, that will be instancied when the app boots, and dispatch it through the container ? Then each class that need a part of config can get it from container ?

Copy link
Sponsor Collaborator

@olivernybroe olivernybroe Sep 15, 2019

Choose a reason for hiding this comment

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

I like that idea. That would work great for when we add more features.

Another idea could be to use a trait, but I'll let you choose, I like both ideas.

@@ -0,0 +1,71 @@
# IDE Integration
Copy link
Owner

@nunomaduro nunomaduro Sep 15, 2019

Choose a reason for hiding this comment

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

I would remove this, and add only:

# IDE Integration

In your `phpinsights.php` file, add the conf...

## Supported IDE 
...
## Unsupported IDE
...

Copy link
Collaborator Author

@Jibbarth Jibbarth Sep 15, 2019

Choose a reason for hiding this comment

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

Do you mean without the prerequisite & troubleshouting part ?

Should i display them as tip or warning instead ?

src/Application/ConfigResolver.php Outdated Show resolved Hide resolved
stubs/config.php Show resolved Hide resolved
Copy link
Owner

@nunomaduro nunomaduro left a comment

Almost there, we just need to address those 2 feedbacks:

src/Application/Console/Formatters/Console.php Outdated Show resolved Hide resolved
src/Domain/Details.php Outdated Show resolved Hide resolved
@Jibbarth Jibbarth force-pushed the feature/ide-url-handler branch 3 times, most recently from 123b558 to 8903d6d Compare Sep 23, 2019
@olivernybroe
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe commented Oct 7, 2019

@Jibbarth When the config file PR is merged in, are you then going to update this PR also?

@Jibbarth
Copy link
Collaborator Author

@Jibbarth Jibbarth commented Oct 7, 2019

@olivernybroe Sure 😊

@Jibbarth Jibbarth force-pushed the feature/ide-url-handler branch from 8903d6d to 6688396 Compare Oct 12, 2019
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe left a comment

Looks good and ready :)

@Jibbarth Jibbarth merged commit 9abd319 into nunomaduro:master Oct 15, 2019
1 check passed
@Jibbarth Jibbarth deleted the feature/ide-url-handler branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants