fix(rustic-babel): disable toolchain when invalid/unneeded#499
fix(rustic-babel): disable toolchain when invalid/unneeded#499yuuyins wants to merge 1 commit intobrotzeit:masterfrom
Conversation
|
i did this fix a while ago and works for me, unsure if it's fit. can't remember why i didn't PR |
psibi
left a comment
psibi
left a comment
There was a problem hiding this comment.
Apart from some minor changes/questions, I think having an additional test would be good too.
rustic-babel.el
Outdated
| (params (remove nil (list "cargo" | ||
| (unless (or (null toolchain) | ||
| (= 0 (string-blank-p toolchain)) | ||
| (equal "+" toolchain)) |
There was a problem hiding this comment.
Any reason why we are doing this check ?
There was a problem hiding this comment.
the (equal "+" toolchain) is in for just in case whatever it might be that no string was given to toolchain
|
working for me with (setq rustic-babel-default-toolchain nil) |
|
forgot about the test i'll have to look into it |
psibi
left a comment
psibi
left a comment
There was a problem hiding this comment.
Thanks, this LGTM. I'm willing to merge this without a test since the updated code is much easier to read, if you are okay with it. :-)
|
@psibi yes, please you can merge it. ah, it seems the existing tests are failing |
|
@yuuyins Sorry, it seems there is some test failures. Can you look into it ? |
| ((or (null rustic-babel-default-toolchain) | ||
| (null (string-blank-p rustic-babel-default-toolchain))) | ||
| nil) |
There was a problem hiding this comment.
this bit doesn't look quite right to me... string-blank-p will return 0 if its an empty string, so null will be false.
I'm not sure if this is related to the failing tests, but if rustic-babel-default-toolchain were set to "" in the test environment, we wouldn't hit nil here and would fall through.
There was a problem hiding this comment.
eq 0 now, thank you so much! if you have resources/time to see what's going on with the tests, that would be wonderful as I'm having to max focus on other stuff rn.
There was a problem hiding this comment.
i will give it a shot, i need to set up all the dependencies locally to run tests, so it may be a bit before i can get to it.
When the user installs Rust tools using a method other than rustup, e.g. using an operating system's package manager, cargo generally has no support for toolchain specification. In such case, the user can then `nil', or `""', so that the respective functions in `rustic-babel' will remove the toolchain from params, i.e. only toolchain has a valid value if Cargo has toolchain support. See also: brotzeit#279 (comment) Fixes brotzeit#498 introduced in 80d05c4 Co-authored-by: Sibi Prabakaran <sibi@psibi.in>
|
@psibi mind approving the workflow for us to determine if the PR still fails or not? |
4 participants
When the user installs Rust tools using a method other than rustup, e.g. using an operating system's package manager, cargo generally has no support for toolchain specification. In such case, the user can then
nil, or"", so that the rescpective functions in `rustic-babel' will remove the toolchain from params, that is only toolchain has a valid value it uses toolchain.See also: #279 (comment), #498