Skip to content
This repository has been archived by the owner on Dec 8, 2020. It is now read-only.

Flush output after every line #17

Merged
merged 3 commits into from Dec 25, 2016
Merged

Conversation

Jascha-N
Copy link
Contributor

@Jascha-N Jascha-N commented Dec 25, 2016

I've added the functionality for reading cargo output one line at a time and printing non-JSON lines to the output pane immediately. Another thing that would be nice is to have lines starting with "{" that fail parsing to still get printed, but it's not that important.

This is my first time working with TypeScript and Node.js, so I hope I didn't mess anything up.

Fixed #16.

@KalitaAlexey
Copy link
Member

Hi @Jascha-N,

Please look at the CONTRIBUTING.md

I want you to execute npm run gulp in order to check for code style violation.
I want you to add "Fixed #16" to the description.

Thanks,
Alexey

@Jascha-N
Copy link
Contributor Author

Already done. :)

@KalitaAlexey
Copy link
Member

I will merge it as soon as I check it.
It will take about 2 hours because currently I am a little busy.
Isn't it a problem?

@KalitaAlexey
Copy link
Member

Already done. :)

Thanks.

@Jascha-N
Copy link
Contributor Author

Actually, do not merge this yet. I did mess something up.

@KalitaAlexey
Copy link
Member

@Jascha-N,
You deleted diagnostics.
This is why there is nothing in the "Problems" panel.
Why did you do this?

@KalitaAlexey
Copy link
Member

KalitaAlexey commented Dec 25, 2016

@Jascha-N,
I like you to use readline. I didn't know about it.

@KalitaAlexey
Copy link
Member

I am going away for one hour.

@Jascha-N
Copy link
Contributor Author

I'll try to fix it. I shouldn't have rushed it. I somehow thought I only removed legacy code.

@KalitaAlexey
Copy link
Member

If I were you I would revert all my changes back and would add only required one.

@Jascha-N Jascha-N reopened this Dec 25, 2016
@Jascha-N
Copy link
Contributor Author

Should be fixed now.

@KalitaAlexey
Copy link
Member

KalitaAlexey commented Dec 25, 2016

@Jascha-N,
It shows errors in the "Problems" panel only at the end.
Could you fix it?
Do you need any guidance how to do that?

@Jascha-N
Copy link
Contributor Author

Well, it was not really the point of this PR, but I guess I can add it.

@KalitaAlexey
Copy link
Member

KalitaAlexey commented Dec 25, 2016

I want you to do that because before your PR problems were printed to the "Problems" panel and a terminal at the same time.
Some people will use only the "Problems" panel.

@Jascha-N
Copy link
Contributor Author

Really? Because I'm 99% sure all of the output was parsed in onGracefullyEnded.

@KalitaAlexey
Copy link
Member

@Jascha-N,
Exactly. You moved output to a terminal from there, but didn't move output to the "Problems" panel.
If it is inconvenient for you. I can merge it as is and implement it myself.

@Jascha-N
Copy link
Contributor Author

Alright, I'm almost done.

@Jascha-N
Copy link
Contributor Author

Should the "Problems" panel be cleared before every cargo command?

@KalitaAlexey
Copy link
Member

Yes, It should.

@Jascha-N
Copy link
Contributor Author

I think it should work.

@@ -564,6 +551,8 @@ export class CommandService {
}

private static createProject(isBin: boolean): void {
this.diagnostics.clear();
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for this since it is a project creation. We don't parse diagnostics here

@KalitaAlexey
Copy link
Member

@Jascha-N,
I need to check it. If everything is okay I will merge it.


let onData = (data: string) => {
output += data;
errors = errors.concat(newErrors);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is more performant than

errors.push(...newErrors);

@KalitaAlexey
Copy link
Member

@Jascha-N,
Okay. It works as I expected.
Do you have time to fix code, which I added comments to?

@Jascha-N
Copy link
Contributor Author

Done.

@KalitaAlexey
Copy link
Member

Thank you.

@KalitaAlexey KalitaAlexey merged commit 521e304 into editor-rs:master Dec 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants