Skip to content

Conversation

@paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Jul 6, 2023

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Looks good!

@kenjis kenjis added the github_actions Pull requests that update GitHub Actions code label Jul 6, 2023
@paulbalandan paulbalandan merged commit 0bf28b2 into codeigniter4:develop Jul 8, 2023
@paulbalandan paulbalandan deleted the no-merge-commits branch July 8, 2023 15:40
@datamweb
Copy link
Collaborator

@paulbalandan , why do we need this feature, just to keep clean history? Or does it have other benefits?

@paulbalandan
Copy link
Member Author

Why added here? I saw you guys asking contributors not use merge commits in PRs so I thought why not automate that. Yes, to keep clean history of PR once merged to develop. You don't want to see the history of develop having Merge develop to ... entries.

@datamweb
Copy link
Collaborator

This strictness will be useful for the repo but annoying for the contributors.
If a user uses git merge (see #789 ), where is it explained how he can pass Detect Merge Commits?
Is there an easier way than git rebase -i or git cherry-pick?

@paulbalandan
Copy link
Member Author

If it is annoying, why are you enforcing it then to contributors manually (saying "Please don't use git merge, use git rebase.")? Isn't that annoying as well?

This is the behavior before and after:
BEFORE: Contributor makes a merge commits, tests all green, maintainer then remarks "don't use merge commits please"
AFTER: Contributor makes a merge commit, all tests pass except this one, it is now outright that he needs to do something to make tests green. Very little intervention needed now from maintainer. He'll just say please fix failing tests.

If you're not comfortable with this addition, please feel free to send a revert PR.

@paulbalandan
Copy link
Member Author

If a user uses git merge (see #789 ), where is it explained how he can pass Detect Merge Commits?

Then make docs for it. I don't understand why you are making a fuss about this when this is already done in the past albeit manually. I just implemented an automated solution.

@kenjis
Copy link
Member

kenjis commented Aug 18, 2023

Then make docs for it.

There is already.

I explained how to do: #789 (comment)

@kenjis
Copy link
Member

kenjis commented Aug 18, 2023

@datamweb We added the Paul's check. But I do not expect every contributor could do git rebase.
So we could merge PRs with the failed check.

But I do not expect all contributors cannot use git rebase.
And I hope more and more contributors can use git rebase, because we really use it everyday,
and clean commit graph is preferable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants