Skip to content

Conversation

@szepeviktor
Copy link
Contributor

@szepeviktor szepeviktor commented Sep 21, 2020

I know - I know - this does not look like a script.

eval "$command_string"
Get_command()
{
echo -n "phpstan --no-interaction --no-progress ${action_command}"
Copy link
Contributor Author

@szepeviktor szepeviktor Sep 21, 2020

Choose a reason for hiding this comment

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

empty action_command (and action_args and action_path below) cause an extra space

Copy link
Contributor Author

@szepeviktor szepeviktor Sep 21, 2020

Choose a reason for hiding this comment

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

AFAIK only action_args is allowed to be empty...

@szepeviktor
Copy link
Contributor Author

We don't have a test :(

@szepeviktor
Copy link
Contributor Author

Please consider running PHPStan in CI.

@g105b
Copy link
Member

g105b commented Sep 22, 2020

Hi @szepeviktor ,

Thank you for your contribution. It looks like there are quite a few changes you're suggesting in this PR. Please could you help me understand the main benefit of the change, so I can review it later today?

Thanks,
Greg.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Sep 22, 2020

I've rewritten the entrypoint from a House of Copy-Paste to a single Bash function.

That is all. You can see the action options in the for loop.

kép

@g105b
Copy link
Member

g105b commented Sep 22, 2020

House of Copy-Paste - that's a new one to me 😄

Thanks, I'm sure this will improve matters greatly. I am a fan of starting projects like this with the least code, then refactoring over time. This looks like a solid refactor, so I will merge once I've tested properly.

@g105b g105b merged commit de9b6dd into php-actions:master Sep 29, 2020
@szepeviktor szepeviktor deleted the patch-2 branch September 30, 2020 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants