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

implement -C/--chdir flag #1479

Merged
merged 4 commits into from
Oct 27, 2023
Merged

implement -C/--chdir flag #1479

merged 4 commits into from
Oct 27, 2023

Conversation

atalii
Copy link
Contributor

@atalii atalii commented Oct 20, 2023

Fixes #1478.

This is my first contribution to any actual code, so expect it to be rough. I imagine for a start we'd want tests for this, as well as actual error messages.

Copy link
Member

@mosteo mosteo left a comment

Choose a reason for hiding this comment

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

This looks good to me, but indeed we would need a new test. For this feature it should be quite simple and I can guide you. First thing is, are you able to run the testsuite now? You can get started with the README in the testsuite folder.

Define_Switch (Config,
Command_Line_Chdir_Target_Path'Access,
"-C=", "--chdir=",
"Run Alire in the given directory");
Copy link
Member

Choose a reason for hiding this comment

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

To refer to the command-line tool proper, better use alr here. (Backticks included as this helps with doc generation later on).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

10b7716 - thanks!

@atalii
Copy link
Contributor Author

atalii commented Oct 23, 2023

I got the test suite running pretty painlessly, and attempted to write a test in fda16e3. I copied init__default-executable and got rid of most of it so as only to test --chdir. Does that look good? I don't think I'd know if I'm missing something, but it seems to work. Thank you so much!

Copy link
Member

@mosteo mosteo left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a minor thing to fix.

Comment on lines 1 to 3
"""
Test "executable" only appears in --bin initializations
"""
Copy link
Member

Choose a reason for hiding this comment

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

Quite common to forget to update this description ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that - fixed in cba2c11, which also tests -C behavior on a non-existent directory. Thanks!

Comment on lines 17 to 18
# Check that it builds and runs
run_alr("--chdir=xxx", "run")
Copy link
Member

Choose a reason for hiding this comment

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

Also, it would be good to have a check after this one that when the target directory doesn't exist, alr exits with error.

@mosteo mosteo merged commit 22a8d77 into alire-project:master Oct 27, 2023
13 checks passed
@mosteo
Copy link
Member

mosteo commented Oct 27, 2023

Thanks, @atalii

@Fabien-Chouteau
Copy link
Member

Hi @atalii, it would be nice to have a user "User-facing changes" entry for this: https://github.com/alire-project/alire/blob/master/doc/user-changes.md

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.

add change directory option to Alire commands
3 participants