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

an include feature for justfiles #1420

Closed
wants to merge 3 commits into from
Closed

an include feature for justfiles #1420

wants to merge 3 commits into from

Conversation

kbknapp
Copy link

@kbknapp kbknapp commented Nov 22, 2022

This design takes all lines that look like this:
@include "relative/path/to/file"
and replaces them with the contents of the pointed-to file. The resulting buffer is passed along to be parsed and executed as before. That's it: no attempt to look at the file or do anything with the contents. This is pure concatenation.

Feel free to tweak as needed!

Implements #1407

examples/included.just Outdated Show resolved Hide resolved
src/loader.rs Outdated
// Parse include directives.
// Include directives are lines that look like:
// #include "relative/path/to/file"
let include_regexp = Regex::new(r#"^#include "([^"]+)"$"#).unwrap();

Choose a reason for hiding this comment

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

Your PR description uses the syntax "@include". Here you use "#include", also in the comment. Let's unify.

Copy link
Author

Choose a reason for hiding this comment

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

Typo from the implementation we used at my work which was #include. I'll fix it up here in a minute.

buf.push('\n');
}

Ok(self.arena.alloc(buf))

Choose a reason for hiding this comment

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

This file currently adds functionality and requires manual validation.
It is missing a minimal automatic test, can you please add one?

src/loader.rs Outdated
Comment on lines 20 to 23
// Parse include directives.
// Include directives are lines that look like:
// #include "relative/path/to/file"
let include_regexp = Regex::new(r#"^#include "([^"]+)"$"#).unwrap();

Choose a reason for hiding this comment

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

Let's extract this logic to a separate, testable function which we call from here.

Copy link

@mihaigalos mihaigalos left a comment

Choose a reason for hiding this comment

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

Nice work, needs a bit of improvement.

@casey casey marked this pull request as draft November 26, 2022 08:23
@casey
Copy link
Owner

casey commented Nov 26, 2022

@mihaigalos This is just a draft, no need to review. I'll probably just use it as a starting point. I'd like to integrate include into the parser, instead of as a preprocessor step.

@mihaigalos
Copy link

@mihaigalos This is just a draft, no need to review. I'll probably just use it as a starting point. I'd like to integrate include into the parser, instead of as a preprocessor step.

That's fine. I would really like this feature.

ceejbot and others added 3 commits November 26, 2022 08:57
* Proof of concept: an include feature for justfiles.

A fast proof that it's straightforward to do a no-fancy-parsing
include feature.

This design takes all lines that look like this:
`@include "relative/path/to/file"`
and replaces them with the contents of the pointed-to file. The resulting buffer
is passed along to be parsed and executed as before. That's it: no attempt to
look at the file or do anything with the contents. This is pure concatenation.

* Satisfy clippy.

Co-Authored-By: kevin@seaplane.io
Co-authored-by: Mihai Galos <mihaigalos@gmail.com>
@kbknapp
Copy link
Author

kbknapp commented Nov 26, 2022

Rebased on the latest master. Thanks @casey! Let me know if there's anything you'd like me to help with or change to make it easier for you.

@casey
Copy link
Owner

casey commented Dec 18, 2022

Okay! I hoped to use this as a basis for a version that would parse included justfiles, and then include the token stream, instead of doing literal text inclusion, but I've been swamped and I know that this has been a long requested feature, so instead of delaying it, let's get this in.

Some comments:

  • This should have a test, maybe in tests/include.rs
  • How about ! instead of @? @ is used for quiet recipes, so ! seems more unambiguous. Also open to other suggestions!
  • How about !include PATH_TO_FILE instead of !include "PATH_TO_FILE"? The argument to include isn't an actual just string, e.g. you can't use single quotes, so the quotes might be more confusing than anything else.
  • This should have require the --unstable flag. I'm open to stabilizing it as-is, since it doesn't conflict with a future version of the feature which uses different syntax, but I'd like to get some testing beforehand.

@neunenak
Copy link
Contributor

neunenak commented Jan 2, 2023

In the interest of getting #1407 closed and this functionality working ASAP, I've made a PR #1470 that solves the same problem. It's loosely based off of this one, but I had to make the code a bit more complicated to handle recursive justfile inclusion.

@mihaigalos
Copy link

  • How about !include PATH_TO_FILE instead of !include "PATH_TO_FILE"? The argument to include isn't an actual just string, e.g. you can't use single quotes, so the quotes might be more confusing than anything else.

Why have any characters before the include? Since if you don't have a colon (:) in the line when specifying an include, you are not defining a recipe.

Good idea on dropping the quotes, they only add noise.

@casey
Copy link
Owner

casey commented Jan 2, 2023

Why have any characters before the include? Since if you don't have a colon (:) in the line when specifying an include, you are not defining a recipe.

Without the ! If the path contains a :, then it's ambiguous whether it's an include or a recipe named include, e.g.:

include :foo

@mihaigalos
Copy link

mihaigalos commented Jan 2, 2023

If the path contains a :

I'm very biased since no ':' in Linux paths afaik, but can imagine C:\ on Windows. Ok. 😅

@casey
Copy link
Owner

casey commented Jan 2, 2023

I'm very biased since no ':' in Linux paths afaik, but can imagine C:\ on Windows. Ok. 😅

Ultimately, linux paths are arbitrary strings, and can include any byte except nulls.

@casey
Copy link
Owner

casey commented Jan 13, 2023

This has been implemented by @neunenak in #1470! For now it's just in master, but I'll cut a release soon so people can play with it.

@casey casey closed this Jan 13, 2023
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.

5 participants