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

[DRAFT] Pre push #1137

Open
wants to merge 11 commits into
base: v2.x
Choose a base branch
from
Open

[DRAFT] Pre push #1137

wants to merge 11 commits into from

Conversation

claudiu-cristea
Copy link

Q A
Branch master for features and deprecations
Bug fix? yes/no
New feature? yes/no
BC breaks? yes/no
Deprecations? yes/no
Documented? yes/no
Fixed tickets comma-separated list of tickets fixed by the PR, if any

New Task Checklist:

  • Are the dependencies added to the composer.json suggestions?
  • Is the doc/tasks.md file updated?
  • Are the task parameters documented?
  • Is the task registered in the tasks.yml file?
  • Does the task contains phpunit tests?
  • Is the configuration having logical allowed types?
  • Does the task run in the correct context?
  • Is the run() method readable?
  • Is the run() method using the configuration correctly?
  • Are all CI services returning green?

@veewee
Copy link
Contributor

veewee commented May 22, 2024

Thanks for the PR.

I'm a bit confused: There is already a draft, so wouldn't it be better to start from 1 PR and try to get that as good as possible?
#1136

@claudiu-cristea
Copy link
Author

This is in a very early stage. Not ready for a review. I didn't want to mess with the previous PR

@claudiu-cristea
Copy link
Author

I wasn't aware of the other PR ☹️

@veewee
Copy link
Contributor

veewee commented May 22, 2024

No problem. Feel free to give your feedback on the initial PR so that we can try to get that one as good as possible.

@claudiu-cristea
Copy link
Author

Hm, I'll have to coordinate with @saidatom. The way we're getting the pushed files list is different

@claudiu-cristea
Copy link
Author

Also, in this PR there's a feature that allows to configure which Git hooks to be initialized. For me, personally, checking on each commit is too agresive. I only want pre push checks. But that's me, others might want on pre commit. Making it customizable makes sense

@veewee
Copy link
Contributor

veewee commented May 22, 2024

True, I think that part could be covered by making it configurable per task basis instead of on a global basis as commented here https://github.com/phpro/grumphp/pull/1136/files#r1608281196.
Not sure how easy it would be to make the 'default' configurable as well - but for BC sense it should at least be pre-commit

@claudiu-cristea
Copy link
Author

@veewee, I have discussed with @saidatom. Until we decide the way to go, I'll continue to improve this. Regarding https://github.com/phpro/grumphp/pull/1136/files#r1608281196, I need to understand more what you are proposing there. You mean, for instance, that phpcs could run on a push while phpstan on commit?

@veewee
Copy link
Contributor

veewee commented May 22, 2024

Regarding https://github.com/phpro/grumphp/pull/1136/files#r1608281196, I need to understand more what you are proposing there. You mean, for instance, that phpcs could run on a push while phpstan on commit?

Indeed: the hook script is activated on both commit and push. Which task is being executed will depend on a per task configuration with default if not set.

@claudiu-cristea
Copy link
Author

claudiu-cristea commented May 26, 2024

@veewee

Indeed: the hook script is activated on both commit and push. Which task is being executed will depend on a per task configuration with default if not set.

I think this is fixed now.

I'm trying now to add some tests for the new feature. It would help a lot to enable GitHub Actions for this PR

_DRAFT__Pre_push_·_phpro_grumphp_896ccd4

@veewee
Copy link
Contributor

veewee commented Nov 26, 2024

We notices this PR became inactive. If you wish to continue, feel free to pick it up again.
If not, we will be closing this PR soon. Thanks for your understanding.

@claudiu-cristea
Copy link
Author

I think the tests were not enabled so I couldn't continue. But I've missed that they were enabled in the meantime

@veewee
Copy link
Contributor

veewee commented Nov 26, 2024

I don't know how to enable it and keep it enabled for this PR.
You might want to run the actions in your local repository instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants