Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(console): add name parameter to #[ConsoleArgument] #617

Merged

Conversation

gturpin-dev
Copy link
Contributor

@gturpin-dev gturpin-dev commented Oct 24, 2024

This PR allow ConsoleArgument Attribute to have a name parameter to override the method parameter name if wanted.

This Fix #588

@coveralls
Copy link

coveralls commented Oct 24, 2024

Pull Request Test Coverage Report for Build 11744835865

Details

  • 25 of 25 (100.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.001%) to 82.497%

Files with Coverage Reduction New Missed Lines %
src/Tempest/Reflection/src/ParameterReflector.php 2 92.0%
Totals Coverage Status
Change from base Build 11734033828: -0.001%
Covered Lines: 7249
Relevant Lines: 8787

💛 - Coveralls

@gturpin-dev gturpin-dev force-pushed the feat/console/console-argument-name branch from 4c839a8 to fe75de0 Compare October 28, 2024 15:57
@brendt
Copy link
Member

brendt commented Oct 29, 2024

I think there's still something missing here. I've added a failing test into your branch, I'd expect this one to work. (\Tests\Tempest\Integration\Console\ConsoleArgumentBagTest::test_name_mapping)

@gturpin-dev
Copy link
Contributor Author

I think there's still something missing here. I've added a failing test into your branch, I'd expect this one to work. (\Tests\Tempest\Integration\Console\ConsoleArgumentBagTest::test_name_mapping)

I don't think this should work if I refer to https://tempestphp.com/docs/console/building-console-commands/

If I'm not wrong the CommandWithArgumentName fixture have a string input and a bool flag so the flag could be update using --new-flag statement in the console input ( which is ok and works for me )

But, the input is a positional parameter, so to update its value we can't name it with --new-name=foo I guess.
I think this test is valid using command-with-argument-name foo --new-flag atm.

But are you asking to update the actual console arg matching to also support the named param ?
If yes I've just misunderstand the request ! ( but does this wouldn't be better in a separate PR ? )
As this one can help us with flag named params and for the default tempest command + help command

@brendt
Copy link
Member

brendt commented Oct 31, 2024

Well in Tempest, all arguments can be passed by name, including positional ones:

final readonly class My
{
    use HasConsole;

    #[ConsoleCommand]
    public function __invoke(string $foo, string $bar): void
    {
        $this->writeln($foo);
        $this->writeln($bar);
    }
}
./vendor/bin/tempest my --bar=a --foo=b
b
a

So I would say that should be supported with this PR before we can merge.

@brendt brendt marked this pull request as draft October 31, 2024 12:21
@gturpin-dev
Copy link
Contributor Author

Ok thanks,
So I guess I've probably break this then because I though it wasn't possible.
I'll check it out.

@innocenzi innocenzi changed the title feat(console): add name parameter to console argument attribute feat(console): add name parameter to #[ConsoleArgument] Nov 8, 2024
@gturpin-dev gturpin-dev force-pushed the feat/console/console-argument-name branch from 773367e to 082695e Compare November 8, 2024 13:56
@gturpin-dev gturpin-dev marked this pull request as ready for review November 8, 2024 14:03
@innocenzi
Copy link
Member

I just pushed a few more changes:

  • fixed a str usage that wasn't imported (you were using the Laravel one locally)
  • fixed the mixing between kebab and camel case, this wasn't coming from your PR specifically but it was confusing before and when trying the PR it felt like it needed immediate fixing
  • added more test coverage for the changes

@innocenzi innocenzi dismissed brendt’s stale review November 8, 2024 15:27

changes were applied

@innocenzi innocenzi merged commit 2a73033 into tempestphp:main Nov 8, 2024
57 checks passed
@gturpin-dev gturpin-dev deleted the feat/console/console-argument-name branch November 12, 2024 12:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Maybe add a name to the ConsoleArgument attribute to override the parameter name
4 participants