Don't show the output channel if checking is an action on save #133

KalitaAlexey · 2017-03-01T19:56:07Z

This PR changes how the extension behave when checking (Cargo: Check) is executed because the user set "rust.actionOnSave" to "check".
Before the PR the output channel was opened if the user hadn't set "rust.showOutput" to false.
After the PR the output channel will not be opened.
It is breaking change, but I think it is the behavior we all expect.

Fixes #129

KalitaAlexey · 2017-03-01T19:58:01Z

@sinkuu @redactedscribe,
What do you think about it?

redactedscribe · 2017-03-01T20:34:09Z

@KalitaAlexey I have no problem with this change. Using "rust.executeCargoCommandInTerminal": true provides more detailed compiler messages than the Output channel anyway.

It also might be a good idea to improve the comment above the rust.showOutput configuration parameter, for example, rust.showOutput has no effect when "rust.executeCargoCommandInTerminal": true. Detailing that build and check do not affect rust.showOutput may be needed also. This information should be in the documentation, but good comments will avoid the need to read through docs for those simple answers.

KalitaAlexey · 2017-03-01T20:38:39Z

@redactedscribe,
Don't we have that it the documentation?

redactedscribe · 2017-03-01T20:49:10Z

@KalitaAlexey The docs do not say that rust.showOutput only functions under certain conditions.

Maybe better than commenting finer details about each configuration parameter in the settings file, you could write "See documentation for details" on params that have certain conditions for them to work.

KalitaAlexey · 2017-03-01T20:51:05Z

@redactedscribe,
Okay. I will do that before merging the PR.

KalitaAlexey · 2017-03-01T20:57:11Z

@redactedscribe,
Can you describe that in the documentation?
You have access to the branch.

redactedscribe · 2017-03-01T23:50:16Z

@KalitaAlexey I will tomorrow.

KalitaAlexey · 2017-03-02T12:28:55Z

doc/legacy_mode/main.md


 ## Configuration Parameters

 ### Show Output

-The `"rust.showOutput"` configuration parameter controls whether the output channel should be shown when [a cargo command is started executing](../cargo_command_execution.md).
+The `"rust.showOutput"` configuration parameter controls whether the Output channel should be shown when [a Cargo command starts executing](../cargo_command_execution.md). All Cargo command other than `build` and `check` trigger this event.


Why capital O in Output?

KalitaAlexey · 2017-03-02T12:30:28Z

doc/legacy_mode/main.md


 ## Configuration Parameters

 ### Show Output

-The `"rust.showOutput"` configuration parameter controls whether the output channel should be shown when [a cargo command is started executing](../cargo_command_execution.md).
+The `"rust.showOutput"` configuration parameter controls whether the Output channel should be shown when [a Cargo command starts executing](../cargo_command_execution.md). All Cargo command other than `build` and `check` trigger this event.


Replace all with any or add s to command.
Actually the PR make different other than you are describing.
I am going to write what the PR does.

KalitaAlexey · 2017-03-02T12:31:19Z

doc/legacy_mode/main.md


-However, it may be changed.
+### Executing Cargo commands in the integrated temrinal


Typo: te mr inal

Why the?

I will replace the with a if it makes more sense. Will fix typo.

@redactedscribe,
We talking about any integrated terminal, not about the only specific terminal.

KalitaAlexey · 2017-03-02T12:33:30Z

doc/legacy_mode/main.md


-Unfortunately, there is no way to parse output of an integrated terminal.
+By default, the extension executes a Cargo command as a child process. It then parses the output of the command and publishes diagnostics. This is useful if you need to run a binary and enter some text.


Replace This is useful with Executing Cargo command in an integrated terminal is useful.
Why do you write a Cargo? I thought the correct variant was "Cargo" because it is the tool's name.

It's either "Executing a Cargo command…" or "Executing Cargo commands…". It doesn't sound right without the a or plural.

@redactedscribe,
I don't mind to keep a Cargo, but replace how I suggested This is useful.

KalitaAlexey · 2017-03-02T12:38:03Z

@redactedscribe,
I have changed the PR's description.

redactedscribe · 2017-03-02T13:08:36Z

@KalitaAlexey Do I need to mention anything about the build action on save? Your PR does not mention that.

KalitaAlexey

A few minor questions.

KalitaAlexey · 2017-03-02T13:23:19Z

doc/legacy_mode/main.md


 The possible values:

 * `true` - the output channel should be shown
 * `false` - the output channel shouldn't be shown

-When `"rust.executeCargoCommandInTerminal"` is set to `true`, `"rust.showOutput"` has no effect.
+The output channel will not be shown under the following conditions:


For me, as not native speaker, it is ambiguous if the output channel will not be shown if all the conditions are met or at least one.

Good point.

KalitaAlexey · 2017-03-02T13:24:03Z

doc/legacy_mode/main.md

+- `"rust.executeCargoCommandInTerminal"` is set to `true`
+- `"rust.actionOnSave"` is set to `"check"`
+
+### Executing Cargo commands in a integrated terminal


Shouldn't that be an instead of a?

Yep, you are correct, Sorry, I am tired today.

KalitaAlexey · 2017-03-02T13:25:50Z

doc/legacy_mode/main.md


 The `"rust.executeCargoCommandInTerminal"` configuration parameter controls whether [a Cargo command should be executed](../cargo_command_execution.md) in an integrated terminal.

-By default, the extension executes a Cargo command as a child process. It then parses the output of the command and publishes diagnostics. This is useful if you need to run a binary and enter some text.
+By default, the extension executes Cargo commands as a child process. It then parses the output of the command and publishes diagnostics. Executing Cargo commands in an integrated terminal is useful if you need to run a binary and enter some text.


For me it sounds weird Cargo commands as a child process.
How about you?

It should be better now.

KalitaAlexey · 2017-03-02T13:34:05Z

Good. Thanks.

KalitaAlexey added 2 commits March 1, 2017 22:49

Deleted default keyword around class.

bee94f2

Fixed issue

a16fc55

Improve Legacy Mode docs and rust.showOutput details

2da4aea

KalitaAlexey commented Mar 2, 2017

View reviewed changes

Make adjustments

3de0992

KalitaAlexey commented Mar 2, 2017

View reviewed changes

Further adjustments

79e9b65

KalitaAlexey merged commit 15a4c55 into master Mar 2, 2017

KalitaAlexey deleted the fix-129 branch March 2, 2017 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't show the output channel if checking is an action on save #133

Don't show the output channel if checking is an action on save #133

KalitaAlexey commented Mar 1, 2017 •

edited

Loading

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017 •

edited

Loading

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017

KalitaAlexey commented Mar 1, 2017

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017

KalitaAlexey Mar 2, 2017

KalitaAlexey Mar 2, 2017

KalitaAlexey Mar 2, 2017

KalitaAlexey Mar 2, 2017

redactedscribe Mar 2, 2017 •

edited

Loading

KalitaAlexey Mar 2, 2017

KalitaAlexey Mar 2, 2017

redactedscribe Mar 2, 2017

KalitaAlexey Mar 2, 2017

KalitaAlexey commented Mar 2, 2017

redactedscribe commented Mar 2, 2017

KalitaAlexey left a comment

KalitaAlexey Mar 2, 2017

redactedscribe Mar 2, 2017

KalitaAlexey Mar 2, 2017

redactedscribe Mar 2, 2017

KalitaAlexey Mar 2, 2017

redactedscribe Mar 2, 2017

KalitaAlexey commented Mar 2, 2017


		However, it may be changed.
		### Executing Cargo commands in the integrated temrinal


		Unfortunately, there is no way to parse output of an integrated terminal.
		By default, the extension executes a Cargo command as a child process. It then parses the output of the command and publishes diagnostics. This is useful if you need to run a binary and enter some text.

Don't show the output channel if checking is an action on save #133

Don't show the output channel if checking is an action on save #133

Conversation

KalitaAlexey commented Mar 1, 2017 • edited Loading

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017 • edited Loading

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017

KalitaAlexey commented Mar 1, 2017

KalitaAlexey commented Mar 1, 2017

redactedscribe commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redactedscribe Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalitaAlexey commented Mar 2, 2017

redactedscribe commented Mar 2, 2017

KalitaAlexey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalitaAlexey commented Mar 2, 2017

KalitaAlexey commented Mar 1, 2017 •

edited

Loading

redactedscribe commented Mar 1, 2017 •

edited

Loading

redactedscribe Mar 2, 2017 •

edited

Loading