Skip to content

implement -C/--chdir flag#1479

Merged
mosteo merged 4 commits into
alire-project:masterfrom
atalii:chdir
Oct 27, 2023
Merged

implement -C/--chdir flag#1479
mosteo merged 4 commits into
alire-project:masterfrom
atalii:chdir

Conversation

@atalii

@atalii atalii commented Oct 20, 2023

Copy link
Copy Markdown
Contributor

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.

@mosteo mosteo left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/alr/alr-commands.adb Outdated
Define_Switch (Config,
Command_Line_Chdir_Target_Path'Access,
"-C=", "--chdir=",
"Run Alire in the given directory");

Copy link
Copy Markdown
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
Copy Markdown
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

atalii commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

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!

@mosteo mosteo left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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 thread testsuite/tests/init/with-chdir/test.py Outdated
Comment on lines +17 to +18
# Check that it builds and runs
run_alr("--chdir=xxx", "run")

Copy link
Copy Markdown
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
@mosteo

mosteo commented Oct 27, 2023

Copy link
Copy Markdown
Member

Thanks, @atalii

@Fabien-Chouteau

Copy link
Copy Markdown
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