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

Add Teun as author #5573

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Add Teun as author #5573

merged 3 commits into from
Dec 13, 2023

Conversation

thomasp85
Copy link
Member

Hi Team

Teun has been instrumental in many of the great things that has happened with ggplot2 in the last year or so. The next feature release is predominantly his work. I see it only fitting to include him as author together with the rest of us. Please approve this PR to show your support or discuss below

There will be a 3 weeks deadline beyond which silence is considered consent

DESCRIPTION Outdated Show resolved Hide resolved
Copy link
Member

@karawoo karawoo left a comment

Choose a reason for hiding this comment

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

Definitely agree 🙂

Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Definitely agree, too! Thank you so much Teun for your great contribution. I didn't expect the development of ggplot2 would make such a wonderful progress in the recent months!

@thomasp85 @hadley
I have one request to you two before this gets merged. Could you update GOVERNANCE.md to reflect the current status? I don't think it describes what the governance is in actual. For example,

  • The current "project lead" is Thomas? I think Thomas has the permission to push to the main branch directly.
  • In my understanding, Teun is not yet a "core developer", but he already has the same permission to approve pull requests and close issues. I don't think the document describes in what case the permission is granted to non-"core developer".
  • Not all the "core developers" are invited in this approval process. (Since this issue is public and the other core developers can react if they have some different opinion, I now feel this point is fine, sorry)

To be clear, it's not that I'm blaming the violation of what is currently defined as the rule. I'm totally fine with it if it's comfortable to you. I think this divergence probably means the current rule is a bit too formal to follow. So, it's time to loosen the rule. Probably, just deleting GOVERNANCE.md is fine.

@thomasp85
Copy link
Member Author

Thanks for raising this @yutannihilation - I agree we have been too lax compared to what we have laid out in governance

@hadley
Copy link
Member

hadley commented Dec 9, 2023

I'd say that I am still the project lead and Teun is a core developer (i.e. this PR should add him to that bullet list too)

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

+10!

It's been a joy to see all the issues I touched years ago slowly but surely get closed over the last few months. Cheers!

Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

Happy to support this PR!

@yutannihilation
Copy link
Member

@hadley
Thanks. Then, I think the document should

  • define a new role for what Thomas currently is. Apparently, he has more power than the normal "core developers". If it's not the "project lead", I think a new role is needed.
  • describe that the project lead can add a "core developer" without getting confirmation from other "core developers". (IMHO, it would be cleaner to define a new role that is not a "core developer" but is temporarily granted the same permission as "core developer" at the "project lead"'s direction. But, probably this is just a matter of preference)

@hadley
Copy link
Member

hadley commented Dec 11, 2023

Why do you think Thomas has more power than other devs? I would say he is acting as my trusted assistant, but I'm not sure that has much impact day-to-day?

@thomasp85
Copy link
Member Author

@yutannihilation as to the second point I made an error there, giving Teun GitHub permissions way before we started the Core Developer inclusion. That's on me and won't happen again. I think the process as described in the governance file is as it should and I should have paid more attention to it

@yutannihilation
Copy link
Member

Why do you think Thomas has more power than other devs?

Simply because Thomas has the permission to push to the main branch directly (e.g. 4fea51b) or to merge a pull request into the main branch without approval (e.g. #5171). I know it's a rare case.

but I'm not sure that has much impact day-to-day?

Yeah, the governance doesn't impact day-to-day. You are right. But, I believe the reason GOVERNANCE.md exists is that we believe the governance does matter even if it's not something we care day-to-day. If not, I think we can just remove GOVERNANCE.md, no?

That's on me and won't happen again. I think the process as described in the governance file is as it should and I should have paid more attention to it

But, isn't it inconvenient to you? I guess it's a common practice to give permission to a Posit intern before they becomes a proper author. I (and probably we all) don't feel the process itself is a problem. I just want the document to match the reality.

Again, I'm not blaming anything. I just want to clarify by utilizing this chance. If it's a burden to you to rewrite the document, I can be calm and just approve. Sorry.

@thomasp85
Copy link
Member Author

I don't think anyone takes your comments in any kind of bad tone - I think it is good that we keep ourself up to the standards that we put forth ❤️.

I think going forward if a new intern/contractor were to be had, we would either start the process earlier (if they had a proven track record already) or wait giving them GitHub permission until such a track record was established and then start the right process

@thomasp85
Copy link
Member Author

I'm admin by side effect through my job, my permissions on this project specifically is the same as everyone else

@yutannihilation
Copy link
Member

I don't think anyone takes your comments in any kind of bad tone - I think it is good that we keep ourself up to the standards that we put forth ❤️.

Thanks!

After some thinking, I'm now convinced it's the best to stick with the current GOVERNANCE.md if following the process is not inconvenient to you. While I still feel there's some room for update, the document is the result of considerable amount of efforts (#2959) and I should respect it.

As my concern is mostly resolved, I'm approving. Thanks for your clarification.

@thomasp85 thomasp85 merged commit a85b06c into main Dec 13, 2023
12 checks passed
@teunbrand
Copy link
Collaborator

Thanks everyone for the kind words and @yutannihilation vigilance on the governance document! ❤️

@yutannihilation
Copy link
Member

Congratulations!

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

Successfully merging this pull request may close these issues.

7 participants