-
Notifications
You must be signed in to change notification settings - Fork 471
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
Implement loading justfile from stdin #1933
base: master
Are you sure you want to change the base?
Conversation
This seems reasonable, but this PR looks a bit too complicated and I think some of the changes are unnecessary. I would create two new SearchConfig variants, |
522568c
to
2b3bed9
Compare
@casey I have added the variants, now we need to change |
There are two options I can think of:
I would try out 1 first and see how it looks and see if it causes any problems, or requires a big refactor. |
5b0c7ef
to
5ac98c0
Compare
Sorry for closing and reopening. It was by mistake. @casey Please have a look at the new commit. I have added a |
This looks like a good approach to me! |
e4e15ae
to
95d51ab
Compare
8de7cd5
to
fb28b25
Compare
Hi @casey I have changed a little the implementation of this PR. Previous approach did not felt right for me. I think the new approach makes more sense. However, I stumbled upon a failing integration test: Let me know what you think about the new changes. Thanks! |
src/search.rs
Outdated
@@ -6,7 +6,7 @@ const PROJECT_ROOT_CHILDREN: &[&str] = &[".bzr", ".git", ".hg", ".svn", "_darcs" | |||
|
|||
#[derive(Debug)] | |||
pub(crate) struct Search { | |||
pub(crate) justfile: PathBuf, | |||
pub(crate) justfile_path: Option<PathBuf>, |
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.
I think this should just be justfile
, since the type is already a path.
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.
I agree. Fixed.
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.
I can't really review this PR yet, because it doesn't actually implement reading justfiles from stdin, it's just kind of preliminaries, which, without seeing the implementation, I'm can't give good feedback on.
src/search.rs
Outdated
/// Returns an empty path if the justfile path is relative and has only one component. | ||
/// Returns an empty path if the justfile path is itself empty. | ||
/// Returns the current working directory if the justfile is not a file on disk. | ||
pub fn justfile_directory(&self) -> PathBuf { |
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.
These functions are kind of footguns. If the justfile was read from stdin, then there is no path or directory to the justfile. I understand that these are kind of compatibility shims, but I think the code just has to be adapted to deal with the fact that the justfile path might be None.
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.
My reasoning is that you still need to know the working directory even if you load the justfile from stdin. On the other hand, many functions don't need to know both the working directory and the optional justfile path. That's what this function does. It abstracts away the justfile directory over the Search
struct. I don't have strong opinion on this, but the abstraction seems reasonable. How do you see it?
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.
working_directory
is fine, and should always be set. It's justfile
and justfile_directory
that I think are the issue, since there is no justfile, and no directory containing it.
f164eb7
to
7aed154
Compare
7aed154
to
aa4785d
Compare
I think it would be nice to have the possibility to pipe justfiles into just, like so:
It would work by detecting a special symbol "-" as the -f / --justfile argument.
Benefits:
The plan:
Search{}
functionality intoSource{}
Source::from_stdin()
Here is the first of the commits, more commits will be added while work progresses.