-
Notifications
You must be signed in to change notification settings - Fork 22
Fixed the way json errors are requested from cargo & rustc #74
Conversation
Ah, I've also added the code to always redirect errors to Atom from |
Closes AtomLinter#69 Also fixed the way envinronment variables were passed to BufferedProcess -- according to [the documentation](https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback) (*`BufferedProcess` is actually a wrapper above node's exec*) they should be passed in an `env` subobject of `options`, but they were passed as raw in options object. Adding to these environment variables accordingly set `RUSTFLAGS` variable, when JSON errors are available, makes cargo tell compiler to output JSON errors. When using `rustc` simply providing `--error-format=json` is enough to make compiler do what we want.
5723802
to
cdc5705
Compare
@@ -21,8 +21,9 @@ class LinterRust | |||
file = @initCmd do textEditor.getPath | |||
curDir = path.dirname file | |||
PATH = path.dirname @cmd[0] | |||
options = JSON.parse JSON.stringify process.env | |||
options.PATH = PATH + path.delimiter + options.PATH | |||
options = new Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you didn't just use {}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arcanemagus the only reason for this is my poor knowledge of CoffeeScript :) I'll fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, every time I see linters still stuck on CoffeeScript I cringe, I'll get to moving them to ES2015 eventually! 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://decaffeinate-project.org/repl/
^ This is great for getting readable code out of the CoffeeScript
That's all I see just looking at the code. |
Changed notifications in dev mode.
@Arcanemagus I've added a cache for 'ability to have JSON errors', fixed 'new object' stuff and the way dev mode notifications look: It is possible to stack them in one error message, if one would prefer it that way. |
As it's already behind a dev mode check, it's probably fine to leave them like that. |
options.cwd = curDir | ||
command = @cmd[0] | ||
args = @cmd.slice 1 | ||
@cachedAbleToJsonErrors = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... is this line really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmist This ensures that the cached value is live only for a one lint process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, exactly
Closes #69
Also fixed the way envinronment variables were passed to BufferedProcess --
according to the documentation (
BufferedProcess
is actually a wrapper above node's exec)they should be passed in an
env
subobject ofoptions
, but they were passed 'as is' in the options object.Adding to these environment variables accordingly set
RUSTFLAGS
variable, when JSON errors are available, makes cargo tell compiler to output JSON errors.When using
rustc
simply providing--error-format=json
is enough to make compiler do what we want.