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

Extract sub-command handling (alr <command>) #806

Merged
merged 12 commits into from Sep 1, 2021

Conversation

Fabien-Chouteau
Copy link
Member

The goal of this big refactoring is to extract the handling of
sub-commands to make it available for other projects.

The SubCommander packages (name open to changes) can be extracted to a
dedicated crate or to AAA.

There is at least one regression which is the support of -X for alr
build.

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.

Neat! I just saw very little things during the review. There is some ugliness that I crafted in there, ouch. Way better having it isolated.

I understand that now alr build simply doesn't see the -X switches. I remember that someone was using that. Perhaps this PR can go in after 1.1 then.

src/alr/alr-commands.adb Outdated Show resolved Hide resolved
src/alr/subcommander.ads Outdated Show resolved Hide resolved
src/alr/subcommander-instance.adb Outdated Show resolved Hide resolved
@Fabien-Chouteau Fabien-Chouteau force-pushed the feat/extra-subcommand-handling branch 2 times, most recently from 6c20d47 to 4512319 Compare August 26, 2021 13:33
@Fabien-Chouteau
Copy link
Member Author

@mosteo I did my final refactoring on this.

Pending checks and reviews, the remaining questions are:

  • Should the name SubCommander change?
  • Where should we put this code? AAA or its own crate?

@Fabien-Chouteau Fabien-Chouteau force-pushed the feat/extra-subcommand-handling branch 2 times, most recently from 6bf4cb4 to 5565a61 Compare August 26, 2021 16:42
@mosteo
Copy link
Member

mosteo commented Aug 27, 2021

Huge effort here, thanks Fabien.

Where should we put this code? AAA or its own crate?

Nowadays I don't see a reason for huge libraries, so I favor splitting things unless they're very thematically related. AAA is going to probably be my last hodgepodge project. So I'd say its own crate.

Should the name SubCommander change?

Besides the camel case (I'd prefer simply Subcommander) I don't have ideas for another name right away...

@Fabien-Chouteau
Copy link
Member Author

Huge effort here, thanks Fabien.

Thank you, I was not expecting to fall in this rabbit hole ^^

Where should we put this code? AAA or its own crate?

Nowadays I don't see a reason for huge libraries, so I favor splitting things unless they're very thematically related. AAA is going to probably be my last hodgepodge project. So I'd say its own crate.

Thinking about this, we have other things like TTY, user input, config files that we can extract from Alire in to a Command Line Interface crate:
For the name?:

  • clic: Command Line Interface Components
  • clic: Command Line Interface Crate
  • Just cli
  • Something else...

Should the name SubCommander change?

Besides the camel case (I'd prefer simply Subcommander) I don't have ideas for another name right away...

Ok for Subcommander.

@mosteo
Copy link
Member

mosteo commented Aug 27, 2021

  • clic: Command Line Interface Components

I like this one a lot, cli is too generic but clic has personality

@onox
Copy link
Contributor

onox commented Aug 27, 2021

I remember that someone was using that. Perhaps this PR can go in after 1.1 then.

I use -X to set some scenario vars (one to switch between AVX and AVX2 code (I run some gcc command in a Makefile to detect which extension is supported)) and another to switch between release and coverage build modes. Although the latter wouldn't be needed if something like #707 is implemented.

@Fabien-Chouteau
Copy link
Member Author

@mosteo I successfully moved Subcommander, TTY and User_Input under a CLIC package hierarchy. Next week I will move CLIC to a dedicated repository in alire-project.

I think Alire.Config can also move to CLIC but there is some refactoring to do.

@mosteo
Copy link
Member

mosteo commented Aug 30, 2021

Thanks for the update, @Fabien-Chouteau .

@onox, the -X parameters are supported again in this PR, so the situation stays as it was.

@Fabien-Chouteau Fabien-Chouteau marked this pull request as ready for review August 31, 2021 07:57
.gitmodules Outdated Show resolved Hide resolved
The goal of this big refactoring is to extract the handling of
sub-commands to make it available for other projects.

The SubCommander packages (name open to changes) can be extracted to a
dedicated crate or to AAA.

There is at least one regression which is the support of -X for alr
build.
The switches are not parsed several times like before.  The global
switches parsing is done once, the command specific switch parsing is
done only on the command arguments.

Handling of -X scenario variable switches is now done with regular
GNAT.Command_Line switch handling.

There is a issue with command arguments that contain spaces, it looks
like they are split in multiple arguments. I don't know why.
The concatenation of arguments and re-splitting with
GNAT.OS_Lib.Argument_String_To_List breaks when arguments have
whitespaces: 'alr' 'show' 'a b c' becomes 'alr 'show' 'a' 'b' 'c'.
This was previously done with a GNAT.Command_Line.Extra package that
exploited the internals of GNAT.Command_Line.

This is more portable because less exposed to changes in the internals
of GNAT.Command_Line.
@mosteo mosteo merged commit 2b91cb0 into master Sep 1, 2021
@mosteo mosteo deleted the feat/extra-subcommand-handling branch September 1, 2021 08:52
@mosteo
Copy link
Member

mosteo commented Sep 1, 2021

Thanks a lot, Fabien, now merged.

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.

None yet

3 participants