Skip to content

Conversation

@MarieSacksick
Copy link
Contributor

@MarieSacksick MarieSacksick commented Jan 24, 2026

Closes #1582

This issue says three things:

  1. We might also consider reformatting a bit the Development outline if needed.
  2. Also would it make sense to rename this to just "Contributing"? Since the context is already within the skrub repo, "Contributing to skrub" might be a bit redundant. (I agreed, and therefore did the change)
  3. we can start by removing "Contributing to skrub" from the drop down menu, since it's already present in the "Development" page.

2 and 3 are tackled.
For 1, I quite like the contributing guide for now, because it's very detailed. Most of what I did is changing from:
image

to:

image

The idea is to go step by step for the contributor. Building the doc locally to me is part of checking the quality of contribution.

^^^^^^^^^^^

Community consensus is key in the integration process. Expect a minimum
of 1 to 3 reviews depending on the size of the change before we consider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually unclear to me: isn't it this instead, since there is a mention of community?

Suggested change
of 1 to 3 reviews depending on the size of the change before we consider
of 1 to 3 reviewers depending on the size of the change before we consider

Copy link
Member

Choose a reason for hiding this comment

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

I would rather have something like "Expect a minimum of 1 to 3 reviews from maintainers..."

Comment on lines 287 to 289
**Before submitting your pull request, ensure that your modifications haven't
introduced any new Sphinx warnings by building the documentation locally
and addressing any issues.**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the number of warnings already present, I think it's very hard check to do and to enforce.
Shouldn't it be removed?

Suggested change
**Before submitting your pull request, ensure that your modifications haven't
introduced any new Sphinx warnings by building the documentation locally
and addressing any issues.**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, other solution would be to create a couple of issues to fix the current warnings in documentation building.

Copy link
Member

Choose a reason for hiding this comment

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

We can do both. From experience silencing all the sphinx warning would be nigh impossible, so I think it makes sense to remove this sentence.

Copy link
Member

@rcap107 rcap107 left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks a lot @MarieSacksick 👍

I left a couple of comments, but aside from that I think it's already a big improvement

CONTRIBUTING.rst Outdated
Comment on lines 290 to 292
..
Given the number of warnings already present, I think it's very hard check to do and to enforce.
Shouldn't it be removed?
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed

^^^^^^^^^^^

Community consensus is key in the integration process. Expect a minimum
of 1 to 3 reviews depending on the size of the change before we consider
Copy link
Member

Choose a reason for hiding this comment

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

I would rather have something like "Expect a minimum of 1 to 3 reviews from maintainers..."

Copy link
Member

@rcap107 rcap107 left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks a lot @MarieSacksick 👍

I left a couple of comments, but aside from that I think it's already a big improvement

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.

DOC - Reorganizing the "Development" section in the website top bar

2 participants