Fix error related to multiple binary targets #25
Conversation
When both 'src/lib.rs' and 'src/main.rs' are present in the project, we default to invoking `cargo rustc`. This command requires an explicit binary name with the `--bin` flag, otherwise it causes the error described in GH-23. This commit adds logic to parse the output of `cargo read-manifest` to determine the target type to check, based on the buffer file name. If the file name matches one of the targets, this target is chosen. Otherwise, the first target is chosen as a default. If the target is a binary, we also extract the binary name and set a new `flycheck-rust-binary-name` variable (see flycheck/flycheck#958) to solve the error. I've tested this work with 'src/main.rs' binaries as well as multiples binaries under 'src/bin'. Fixes GH-23, and adds logic to tackle GH-8.
|
Thank you very much, that's a really awesome change! 👏 I'm going to merge the Flycheck PR soon, and meanwhile review this one. |
| name)." | ||
| (let* ((manifest (let ((json-array-type 'list)) | ||
| (json-read-from-string | ||
| (shell-command-to-string "cargo read-manifest")))) |
There was a problem hiding this comment.
Please don't use shell-command-to-string. Going through the shell comes with all kinds of troubles, from poor error reporting to messy output, etc.
Use (with-temp-buffer (call-process "cargo" nil (current-buffer) nil "read-manifest") (json-read)) instead. That avoids the shell and gives us direct access to the process which is more efficient and easier to debug.
There was a problem hiding this comment.
Ok, skipping the shell is indeed best.
Out of curiosity, would with-output-to-string work as well, or are there downsides?
- Fix docstring backquotes - Use `call-process` instead of `shell-command-to-string` - Use `let-alist` to lookup alists
| @@ -103,7 +135,8 @@ Flycheck according to the Cargo project layout." | |||
| (interactive) | |||
| (when (buffer-file-name) | |||
| (-when-let (root (flycheck-rust-project-root)) | |||
| (let ((rel-name (file-relative-name (buffer-file-name) root))) | |||
| (let ((rel-name (file-relative-name (buffer-file-name) root)) | |||
| (target (flycheck-rust-find-target (buffer-file-name)))) | |||
There was a problem hiding this comment.
As we don't need target itself but only its components, please use pcase-let instead of let to destructure the pair immediately. I think the binding form would be ((,target-type . ,target-name) (flycheck-rust-find-target …))`.
Makes no difference semantically, but it's much easier to read :)
|
@fmdkdd Great, thanks for refactoring so quickly. I took a closer look and found two other possible refactorings that make the code simpler. I'm sorry to be so picky, but may I ask you to update your PR again? By the way, please leave a comment when you push new changes to this PR. Github doesn't notify for updates to a PR, so I won't get to know when you're done otherwise. Many many thanks. I'm excited to merge this; I think it'll really improve the Rust experience with Flycheck. |
- Use `seq-find` instead of `dolist` - Add `seq` as dependency - Use `pcase-let` to avoid unnecessary binding
|
@lunaryorn Updated. That's no pickiness, that's attention to detail ;) I'm all for simpler code, and I like learning about new constructs that can make it more straightforward, so I welcome your review. Do you know of any resources for learning about libraries such as In any case, it's you who should be thanked for Flycheck! |
|
@fmdkdd I don't know whether there's a comprehensive list of these "modern" libraries. I guess we should write one ;) I discovered these either by reading other people's code or by following blogs (http://endlessparentheses.com is a good resource). I'll review again soon. |
|
@fmdkdd LGTM now. Did you test it? Does the updated code work? If yes, I'll merge. Would you like to join Flycheck to help maintain our Rust support? |
|
@lunaryorn Tested it, and it works for me. With the accompanying update to Flycheck, this fixes #23. The only downside currently is the choice of default target. We might not pick the "right" one in complex setups, as explained by @tomjakubowski in #23. But then, we can wait and see if that poses a problem for users. In the meantime, there's always the new Flycheck variable to override whatever target we find here. I think I can commit some time for the foreseeable future. Glad to help :) |
|
@fmdkdd Great. I'll add you to our team tomorrow. I'll merge the PR as soon as the corresponding Flycheck PR is merged. It's all prepared, just a small question remaining 😉 |
|
@fmdkdd I've merged this PR. Thank you very much, it's awesome work and I think it'll really improve Rust support for Flycheck. 👏 👍 |
|
Thank you for your review. Hopefully I didn't break anyone else's setup. |
When both 'src/lib.rs' and 'src/main.rs' are present in the project, we default to invoking
cargo rustc. This command requires an explicit binary name with the--binflag, otherwise it causes the error described in GH-23.This commit adds logic to parse the output of
cargo read-manifestto determine the target type to check, based on the buffer file name. If the file name matches one of the targets, this target is chosen. Otherwise, the first target is chosen as a default.If the target is a binary, we also extract the binary name and set a new
flycheck-rust-binary-namevariable (see flycheck/flycheck#958) to solve the error.I've tested this work with 'src/main.rs' binaries as well as multiples binaries under 'src/bin'.
Fixes GH-23, and adds logic to tackle GH-8.