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

PR Attribution #424

Closed
senti-mark-monteiro opened this issue Sep 11, 2024 · 3 comments
Closed

PR Attribution #424

senti-mark-monteiro opened this issue Sep 11, 2024 · 3 comments

Comments

@senti-mark-monteiro
Copy link

I have submitted two PRs to this repo (#414, #416) whose code changes were accepted. However, rather than just merge the PRs into the repository, my code was copied and then included in different PRs by a different author.

I appreciate all the hard work the maintainers are putting into keeping this project running, and I understand that this workflow is probably faster for maintainers to work with. However, the there are several issues that arise when doing this:

  • All the original information/comments from the original PR are not linked to the repository commit history. When you merge a PR normally, the merge commit will link to the PR which can provide valuable information for someone reviewing the code later on
  • Code is not properly reviewed when a lot of unrelated changes are grouped together into one large PR
  • The original author is not credited with the work, which in not a nice experience for those volunteering time to submit code
  • Statistics regarding the project's health are negatively affected. In particular, the list of contributors is going to look smaller than it actually is. This is a common statistic that people look at to judge the health of open source projects
  • Future contributors may be turned off from participating if they see that PRs are regularly closed and not merged

Again, I appreciate the work the maintainers are doing here. I would just like to point out that this workflow is definitely non-standard and in my opinion is not healthy for an open source project that hopes to pull in contributors. Perhaps this workflow is just being used for now to get v1.0 out the door, which is also understandable. In the end I don't feel strongly either way, but I thought it might be helpful to raise this as a concern.

@safwansamsudeen
Copy link
Collaborator

Thank you for your expressing your genuine thoughts respectfully!

I do very much agree with you, especially with this point:

The original author is not credited with the work, which in not a nice experience for those volunteering time to submit code

The problem is that we had a huge PR in April that changed a lot of things - this wasn't merged until yesterday. Thus, all the PRs since then have a lot of conflicts to manage, which makes it a nuisance to merge. As many of them are minor changes, it's much easier for me to just redo them on the current state of the repo.

Any PRs you submit after today should be merged normally.

Something similar happened in March - after a month long of build-up PRs, we started merging PRs normally.
image

Code is not properly reviewed when a lot of unrelated changes are grouped together into one large PR

This is a good point, and one we'll consider. My only defense might be that we're doing these only in the cases of major collecting changes - to ensure that everything works together before changing.

I would just like to point out that this workflow is definitely non-standard and in my opinion is not healthy for an open source project that hopes to pull in contributors. Perhaps this workflow is just being used for now to get v1.0 out the door, which is also understandable.

It's not even v1 - it's just about the conflicts yesterday, and things should proceed normally from now on!

Again, thanks for challenging our practices - constructive criticism like yours are exactly what makes OSS projects like these thrive.

I apologize for the inconvenience of not being attributed - it's quite frustrating for me too. If it's any consolation, I'll try to compile a list of people who helped build the big PR. Again, more importantly - this should not happen again. Due to a series of unpredicted events in May (I was off at camp, and a team lead who was supposed to merge this went on a sabbatical), we left a huge PR dangling - a terrible idea.

@mark-monteiro
Copy link

Thanks for the thoughtful response! That all sounds good to me, totally understand the need to get those large PRs across the finish line. And thanks again for your hard work on the project, the new version is looking good 👍

@safwansamsudeen
Copy link
Collaborator

It's my pleasure, and things are back to normal now, I'm glad to say!

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

No branches or pull requests

3 participants