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

feat: Argument Bag, aliases, help lines #1

Merged
merged 8 commits into from
Apr 9, 2024
Merged

feat: Argument Bag, aliases, help lines #1

merged 8 commits into from
Apr 9, 2024

Conversation

przemyslaw-przylucki
Copy link
Contributor

#1 hell yeah

Adds additional functionality to the console app by creating a ConsoleArgumentBag.

The bag is a class responsible for:
a) holding all of the arguments passed to function
b) creating ConsoleInput, which is a prepared list of available arguments with values. This can throw if the count of arguments doesn't match handler's method signature

It also adds support for:

  • aliasing commands
  • aliasing arguments

I've decided to go with help as an array, I think that for more advanced CLI apps, this could be benefitial.

I think we should have description and help as two separate entities.

Help would be responsible for providing information on how different values of an argument can affect execution, whereas description just expands on the name or illustrates the use-case.

src/ConsoleArgumentBag.php Outdated Show resolved Hide resolved
src/ConsoleArgumentBag.php Outdated Show resolved Hide resolved
src/ConsoleApplication.php Outdated Show resolved Hide resolved
src/ConsoleArgument.php Outdated Show resolved Hide resolved
src/ConsoleArgument.php Outdated Show resolved Hide resolved
src/ConsoleArgument.php Outdated Show resolved Hide resolved
src/ConsoleCommand.php Outdated Show resolved Hide resolved
src/ConsoleCommand.php Outdated Show resolved Hide resolved
src/ConsoleInputArgument.php Show resolved Hide resolved
@przemyslaw-przylucki przemyslaw-przylucki changed the title Argument Bag, but nicest feat: Argument Bag, aliases, help lines Apr 6, 2024
Copy link
Member

@brendt brendt left a comment

Choose a reason for hiding this comment

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

Super nice! Some more nitpicks, but this is gonna be a great PR :D

src/ConsoleApplication.php Show resolved Hide resolved
src/ConsoleArgumentBag.php Outdated Show resolved Hide resolved
src/ConsoleArgumentDefinition.php Show resolved Hide resolved
src/ConsoleCommand.php Outdated Show resolved Hide resolved
src/ConsoleInputBuilder.php Outdated Show resolved Hide resolved
src/ConsoleInputBuilder.php Show resolved Hide resolved
src/ConsoleInputBuilder.php Outdated Show resolved Hide resolved
*/
unset($passedArguments[$idx]);
unset($definitionArguments[$definitionIdx]);
continue 2;
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to not having to rely on continue 2. Maybe it's an indication that this nested loop should be refactored to a separate (private) function for simplicity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - had to use reference which I don't love - if you have better idea lemme know

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we determine whether we should unset the array key based on the output of resolveArgument?

If it's null, we won't unset?

@przemyslaw-przylucki
Copy link
Contributor Author

przemyslaw-przylucki commented Apr 8, 2024

@brendt lemme know what you think!

Also lemme know if contributions this early are helpful for you - I love this project and would love to help as much as possible, but I don't want to come off as intrusive. So if it's counterproductive atm because of the stage lemme know - do not want to disrupt your vision. Otherwise - excited to add a bunch of goodies after this gets merged 🙇

src/ConsoleCommandDefinition.php Show resolved Hide resolved
) {
}

public static function fromString(string $argument, int $position): ConsoleInputArgument
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering: do we need to keep track of the position? I'd assume the position is implied because we're looping over all input in order?

@brendt
Copy link
Member

brendt commented Apr 9, 2024

Both my comments aren't blockers btw, just curious about your input.

Also lemme know if contributions this early are helpful for you - I love this project and would love to help as much as possible, but I don't want to come off as intrusive. So if it's counterproductive atm because of the stage lemme know - do not want to disrupt your vision. Otherwise - excited to add a bunch of goodies after this gets merged 🙇

They absolutely are helpful, no worries! As long as you're willing to put up with me sometimes requiring a couple of days before being able to review code, and my reviews in general, we're totally fine :)

@brendt brendt merged commit 100577b into tempestphp:main Apr 9, 2024
1 of 4 checks passed
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.

2 participants