Support multiple recipe search path arguments #1475

neunenak · 2023-01-04T22:48:17Z

Allow multiple search-path recipe arguments if the recipe search path is the same for all of them, and raise an error message if they have conflicting search paths.

This means that if you have a subdirectory subdir with a Justfile containing recipe_a and recipe_b, you can now invoke just as just subdir/recipe_a subdir/recipe_b, and have it correctly run recipe_a and recipe_b from that Justfile (the current behavior is that it will try to parse "subdir/recipe_b" as a recipe name in and of itself and fail).

If you try to specify multiple separate Justfiles using this syntax (e.g. just subdir1/recipe_a subdir2/recipe_b where subdir1 and subdir2 contain different Justfiles), just will fail with an error message.

casey

I think this is totally reasonable. See review for comments.

src/config_error.rs

src/positional.rs

casey · 2023-01-05T00:14:13Z

src/positional.rs

@@ -225,4 +252,35 @@ mod tests {
 search_directory: None,
 arguments: ["a", "a=b"],
 }
+
+ test! {


I think these would be better as integration tests. I'd also add one that tries other args, like ., ./foo, ../foo

Hm, any reason in particular an integration test would be better? Seems to me like a unit test can exercise the new functionality here, and that it's better to use a unit test than an integration test all else being equal because unit tests are simpler to set up and modify.

With unit tests, you have to think about whether or not the unit test captures all aspects of the feature or change. In this case, the error test doesn't check the formatting of the error message, which an integration test would. But, engaging my brain for a second, I think it's fine here.

jpbochi · 2023-01-05T09:50:25Z

@neunenak I'm not acquainted with the source code here, nor with rust, but I'm interested in this change. Would you mind writing a higher level example[s] of what this change does? 🥺

neunenak · 2023-01-05T19:38:19Z

@neunenak I'm not acquainted with the source code here, nor with rust, but I'm interested in this change. Would you mind writing a higher level example[s] of what this change does? pleading_face

Updated the description to hopefully make the intent of this change clearer.

jpbochi · 2023-01-06T09:03:43Z

@neunenak Thanks! That makes sense to me. I really like the compromise you found.

Should you also update the README.md part about "Invoking justfiles in Other Directories"? Currently, it says that a / will only matter in the first argument. With your change, it will matter in other arguments, too.

casey

Looks good! I would:

Add an integration test that checks the formatting of the error message.
Update the readme

casey · 2023-01-06T21:44:10Z

src/positional.rs

@@ -225,4 +252,35 @@ mod tests {
 search_directory: None,
 arguments: ["a", "a=b"],
 }
+
+ test! {


With unit tests, you have to think about whether or not the unit test captures all aspects of the feature or change. In this case, the error test doesn't check the formatting of the error message, which an integration test would. But, engaging my brain for a second, I think it's fine here.

Allow multiple search-path recipe arguments if the recipe search path is the same for all of them, and raise an error message if they have conflicting search paths.

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

casey

Nice, reworded the readme a little, but otherwise LGTM!

This reverts commit d499227.

- Revert #1475 - Add a test that arguments with different path prefixes are allowed

neunenak mentioned this pull request Jan 4, 2023

Counter-intuitive behavior when invoking multiple recipes from justfile in subdirectories #1473

Closed

neunenak force-pushed the fix-multiple-positionals branch 2 times, most recently from e458503 to 55667f3 Compare January 4, 2023 22:55

casey requested changes Jan 5, 2023

View reviewed changes

neunenak requested a review from casey January 5, 2023 01:02

casey requested changes Jan 6, 2023

View reviewed changes

neunenak and others added 9 commits January 11, 2023 10:37

Support multiple recipe search path arguments

134380c

Allow multiple search-path recipe arguments if the recipe search path is the same for all of them, and raise an error message if they have conflicting search paths.

Update src/config_error.rs

545c854

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Update src/positional.rs

6fba17c

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Update src/positional.rs

2585a9c

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Update src/positional.rs

d465107

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Update src/config_error.rs

5171185

Co-authored-by: Casey Rodarmor <casey@rodarmor.com>

Fix variant name

1d40fbe

Use values.next() instead of continue to advance loop

037a939

Update readme

cd0cd8d

neunenak force-pushed the fix-multiple-positionals branch from 019c786 to cd0cd8d Compare January 11, 2023 18:42

neunenak requested a review from casey January 11, 2023 20:39

Integration test

104823b

neunenak force-pushed the fix-multiple-positionals branch from ac4d25f to 104823b Compare January 12, 2023 04:23

casey added 3 commits January 11, 2023 22:54

Rename tests

dc13a43

tweak

388d1c0

Merge branch 'master' into fix-multiple-positionals

a60f3bb

casey enabled auto-merge (squash) January 12, 2023 07:02

casey approved these changes Jan 12, 2023

View reviewed changes

casey merged commit d499227 into casey:master Jan 12, 2023

neunenak deleted the fix-multiple-positionals branch January 12, 2023 07:08

neunenak mentioned this pull request Jan 24, 2023

bug: new "conflicting path arguments" error in 1.12.0 #1523

Closed

neunenak added a commit to neunenak/just that referenced this pull request Jan 25, 2023

Revert "Allow matching search path arguments (casey#1475)"

f0d04a5

This reverts commit d499227.

neunenak mentioned this pull request Jan 25, 2023

Revert "Allow matching search path arguments (#1475)" #1528

Merged

casey pushed a commit that referenced this pull request Jan 25, 2023

Ignore additional search path arguments

7b55fd0

- Revert #1475 - Add a test that arguments with different path prefixes are allowed

casey pushed a commit that referenced this pull request Jan 25, 2023

Ignore additional search path arguments (#1528)

1a5f07c

- Revert #1475 - Add a test that arguments with different path prefixes are allowed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple recipe search path arguments #1475

Support multiple recipe search path arguments #1475

neunenak commented Jan 4, 2023 •

edited

Loading

casey left a comment

casey Jan 5, 2023

neunenak Jan 5, 2023

casey Jan 6, 2023

jpbochi commented Jan 5, 2023

neunenak commented Jan 5, 2023

jpbochi commented Jan 6, 2023

casey left a comment

casey Jan 6, 2023

casey left a comment

Support multiple recipe search path arguments #1475

Support multiple recipe search path arguments #1475

Conversation

neunenak commented Jan 4, 2023 • edited Loading

casey left a comment

Choose a reason for hiding this comment

casey Jan 5, 2023

Choose a reason for hiding this comment

neunenak Jan 5, 2023

Choose a reason for hiding this comment

casey Jan 6, 2023

Choose a reason for hiding this comment

jpbochi commented Jan 5, 2023

neunenak commented Jan 5, 2023

jpbochi commented Jan 6, 2023

casey left a comment

Choose a reason for hiding this comment

casey Jan 6, 2023

Choose a reason for hiding this comment

casey left a comment

Choose a reason for hiding this comment

neunenak commented Jan 4, 2023 •

edited

Loading