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

DOC: rephrase the role of core devs #28912

Merged
merged 18 commits into from
May 2, 2024

Conversation

GaelVaroquaux
Copy link
Member

A slight adjustment in phrasing and framing to give more visibility to all contributors, and to avoid suggesting that core devs are the only authors.

This PR is done to implement the discussion of today's meeting.

Copy link

github-actions bot commented Apr 29, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 464c81f. Link to the linter CI: here

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

Co-authored-by: Jérémie du Boisberranger <[email protected]>
doc/about.rst Outdated
Comment on lines 32 to 33
roles, however a complete list of contributors can be found `on github
<https://github.com/scikit-learn/scikit-learn/graphs/contributors>`__.
Copy link
Member

Choose a reason for hiding this comment

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

that page only shows top 100 people, which at this point means minimum 24 PRs merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rephrased slightly to be more nuanced

Co-authored-by: Adrin Jalali <[email protected]>
GaelVaroquaux and others added 3 commits April 30, 2024 17:40
GaelVaroquaux and others added 5 commits April 30, 2024 18:38
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @GaelVaroquaux.

LGTM modulo a few nitpicky comments, questions, and a suggestion regarding one element of yesterday's discussion (i.e the rewording of "Authors").

@@ -22,11 +22,22 @@ Governance
The decision making process and governance structure of scikit-learn is laid
out in the :ref:`governance document <governance>`.

Authors
-------
.. _authors:
Copy link
Member

Choose a reason for hiding this comment

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

Should this tag be adapted? Shall we also credit the original academical authors of scikit-learn and its associated publication?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can adapt this tag if you think that it is important. I did not think it was, as it's purely an internal tag, and I was worried of doing my git grep wrong and thus missing some places pointing there.

I'm not in favor of crediting the original academic authors and the associated publication more than they already are: this list reflect the people involved in the first couple of years, which is a small fraction of the total road that got scikit-learn to where it is today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, regarding the tags, I added it to make sure that old html links that point to the anchor "#authors" in html still worked.

I'm relearning an old lesson: each time one does a non obvious thing, it should come with a comment :)

doc/about.rst Outdated
................

The following people are currently maintainers, in charge of
consolidating scikit-learn's development and maintenance:

.. include:: authors.rst
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to rename authors.rst?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

I hope that I did not screw something up here. I guess that we'll know next time we update the contributor list, eg when updating the emeritus

@adrinjalali adrinjalali merged commit 8f96794 into scikit-learn:main May 2, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants