Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add just_executable() function #775

Merged
merged 5 commits into from
Mar 28, 2021
Merged

Conversation

bew
Copy link
Contributor

@bew bew commented Mar 27, 2021

Closes #774

I did not add tests because I couldn't find tests for the other functions, but if you need them I can add those.
EDIT: nevermind, I've found them, will try to make a test

@bew bew force-pushed the add-just_executable-func branch from 2581c64 to ebe4194 Compare March 27, 2021 17:43
@bew
Copy link
Contributor Author

bew commented Mar 27, 2021

For the windows build, I'm not sure how to fix this:

---- misc::test_just_executable_function stdout ----
thread 'misc::test_just_executable_function' panicked at 'assertion failed: `(left == right)`: bad output

Diff < left / right > :
 Output {
<    stdout: "Executable path is: D:ajustjusttargetdebugjust.exe\n",
>    stdout: "Executable path is: D:\\a\\just\\just\\target\\debug\\just.exe\n",
     stderr: "",
     status: 0,
 }

', tests\test.rs:123:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Do I need to change the value returned by the function to make \ visible? Like using path.display() before returning or sth like that?

still new to rust

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Nice, thank you for the PR! I made comments regarding style and formatting, but otherwise this looks great.

tests/misc.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
src/function.rs Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
@bew
Copy link
Contributor Author

bew commented Mar 28, 2021

I changed the PR following your comments!
I don't know how you prefer to do reviews, if you want me mark your comments as resolved or not, so I didn't do it to be safe :)
(I usually prefer this anyway, so you can review the changes and mark-resolved your comments as you go)

Edit: windows test is not better 😕

@casey
Copy link
Owner

casey commented Mar 28, 2021

I don't know how you prefer to do reviews, if you want me mark your comments as resolved or not, so I didn't do it to be safe :)

Either way is fine with me!

Edit: windows test is not better 😕

Hmmm, try this:

printf '%s\n' '{{just_executable()}}'

The behavior of echo with respect to backslashes can be inconsistent. Printf is consistent across platforms, and does not interpret escape sequences in arguments after the first.

@bew
Copy link
Contributor Author

bew commented Mar 28, 2021

I got some weird messages with that printf line, and I had a hard time finding the reason ^^ With errors ranging from Unexpected closing delimiter '}' to Unterminated string or a crash of the test function..

The solution was to not use a \n but a \\n to ensure the newline is not included in the justfile, but only in the recipe' output.

Let's see how Windows work now
🎉 🎉 CI is finally green 🎉 🎉

@casey casey merged commit 13e9f40 into casey:master Mar 28, 2021
@casey
Copy link
Owner

casey commented Mar 28, 2021

Nice, merged! Thank you for the idea, and the implementation.

I just released v0.8.6 that contains this change.

@bew bew deleted the add-just_executable-func branch March 29, 2021 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function to get just' executable path (for when just is not in $PATH)
2 participants