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

Add an option to suppress popping up the output panel for a failed run #247

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

orta
Copy link
Contributor

@orta orta commented Oct 22, 2017

Should be enough to close #168

Allows a user to opt out of the output channel being popped up, TBH it might be worth thinking about making this be the default and allow opting in to showing the output channel on an error. I can't imagine that the errors are inside prettier any more, and I'm willing to bet that VS Code makes it pretty obvious that your source code isn't compiling correctly. Plus you'll see the "prettier x" in the bottom right.

@CiGit CiGit requested review from RobinMalfait and removed request for RobinMalfait October 31, 2017 09:03
@CiGit
Copy link
Member

CiGit commented Oct 31, 2017

Hey, thanks for your PR.

I don't think an option for that is a good move.
We could discuss to remove the automatic opening. It's definitely not in prettier's scope to lint your code. prettier : X may be enough to say something went wrong
I'll let @RobinMalfait decide on that one ;-)

As I'm working on #243 there are things which will have to change anyway. (Or I'll have to do some dirty checks to keep the same functionalities)

@RobinMalfait
Copy link
Contributor

Hey! Thanks for the PR!

I have been annoyed by the popup itself, especially because it doesn't close when the formatting is correct.

The changes I have requested should result in remove some code (Yay!)

Thanks for the PR!

Copy link
Contributor

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

What I would do for this PR is the following:

  • Do not add an extra option (options are not fun and hard to maintain)
  • Always write to the output log (I think this is already the case)
  • Do not show the popup on first error

@orta orta force-pushed the supress_output_channel branch 3 times, most recently from 3a5f3af to 555a78b Compare October 31, 2017 12:00
@orta
Copy link
Contributor Author

orta commented Oct 31, 2017

Alright, agreed - this does that 👍 now, much simpler

@RobinMalfait
Copy link
Contributor

Perfect!
@CiGit can you verify? Code LGTM

@CiGit
Copy link
Member

CiGit commented Oct 31, 2017

@RobinMalfait

Always write to the output log (I think this is already the case)

did you mean https://github.com/prettier/prettier-vscode/blob/master/src/PrettierEditProvider.ts#L161 ?
onWorkspaceRootChange is quite embarrassing for #243

Code looks good.
+2 -13 lines is a really good fix

@RobinMalfait
Copy link
Contributor

I was talking about

addToOutput(warningMessage);

I didn't know the warning message was still used?

@CiGit
Copy link
Member

CiGit commented Oct 31, 2017

Well it seems to be used. And It can be removed I think.

@orta
Copy link
Contributor Author

orta commented Nov 1, 2017

^ I can't tell if this is you two asking me to amend this PR

@RobinMalfait
Copy link
Contributor

@orta Go ahead 😄

Copy link
Member

@CiGit CiGit left a comment

Choose a reason for hiding this comment

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

I'm fine with that. We can remove last warning in a later PR (I'm actually working on it)

@CiGit CiGit merged commit e5d4392 into prettier:master Nov 3, 2017
@RobinMalfait
Copy link
Contributor

@CiGit Cool, thanks for the PR @orta !

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.

no more popup warnings :(
3 participants