#39367 DOC: split contributing.rst

[David-dmh]

• closes DOC: split Contributing page #39367

I created a new PR due in an attempt to close #39367. Previously I had issues regarding environment setup but all seems to be working now thanks to Docker. I ran the linting tests prior to submitting this PR.

[David-dmh]

Could any of the project members assist me with resolving this PR? I am kind of at a loss as to how to resolve the last few checks. It seems to be a build issue but having checked the formatting and rst syntax all looks well to me. The new files I created followed the template of the existing files. I used a Docker container in VS Code to build the docs.

[David-dmh]

Thanks Marco, looking forward to resolving this issue for the project.

[dsaxton]

@David-dmh I think the issue may be here:

/home/runner/work/pandas/pandas/doc/source/development/contributing.rst:162: WARNING: undefined label: contributing.documentation
/home/runner/work/pandas/pandas/doc/source/development/contributing_codebase.rst:837: WARNING: undefined label: contributing.documentation

We're still referencing the contributing.documentation section, but it's been moved to its own file. Try referencing the new file similar to how code_style.rst is on line 685 of contributing.rst from master.

@MarcoGorelli does this seem right to you? I'm partially making an educated guess here as I'm not terribly familiar with Sphinx myself.

[MarcoGorelli]

Hi @David-dmh - can you merge master and address #40130 (comment) please?

[dsaxton]

@David-dmh Looks like just a couple pre-commit errors (trailing whitespace and spelling) but otherwise green

[David-dmh]

@dsaxton ok good, I'm busy working on the pre-commit checks now.

[David-dmh]

I fixed the spelling however could not identify any trailing whitespaces. I used an extension in VS code to highlight and none came up for the files I added/edited. Is there perhaps a way of determining the specific file(s) or line(s) resulting in the check failing?

[MarcoGorelli]

Hi @David-dmh - you can run pre-commit on the files you've modified, see https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#pre-commit

[MarcoGorelli]

Thanks @David-dmh for having worked on this

I realise the original issues was to make a page for Docker separate from the rest, but I think I'd prefer to have a page for "setting up a development environment" which has subsections "with Docker", "with Miniconda", and "with pip" - @afeld thoughts?

[MarcoGorelli]

Rather than just removing this section, I'd suggest to put a link to the "contributing outside of docker" page

[MarcoGorelli]

Actually, if we just have a separate "setting up a development environment" page, then can just be a single link to that

[MarcoGorelli]

@dsaxton thoughts on this? It just seems a bit strange to me to special-case Docker

[dsaxton]

@MarcoGorelli I think that makes sense. Something like contributing_environment.rst which includes both Docker instructions as well as non-Docker, rather than env_outside_docker.rst and then still putting Docker environment instructions in main contributing.rst?

[David-dmh]

With regards to fixing the last trailing whitespace error, I ran pre-commit for the entire doc/source/development dir however I passed the pre-commit checks. The issue seems to be arising from the unmodified portion of the project files. I will then need to run pre-commit elsewhere to isolate the issue.

[MarcoGorelli]

Can you post the command you ran, along with output?

[dsaxton]

With regards to fixing the last trailing whitespace error, I ran pre-commit for the entire doc/source/development dir however I passed the pre-commit checks. The issue seems to be arising from the unmodified portion of the project files. I will then need to run pre-commit elsewhere to isolate the issue.

Check out line 150 of contributing.rst, it looks like that is where the issue is

[dsaxton]

@David-dmh Don't worry about the other pre-commit failure, it's unrelated and there's already a fix for it

[David-dmh]

@David-dmh Don't worry about the other pre-commit failure, it's unrelated and there's already a fix for it

Ok, so apart from the new proposals it looks good?

[dsaxton]

Ok, so apart from the new proposals it looks good?

I think so, to be safe we probably want to wait for the CI fixes to be merged and then merge those in here

[MarcoGorelli]

Thanks for updating - current CI failures are unrelated

[afeld]

Sorry for slow response. My thinking with "splitting out sub-pages for…setting up an environment outside of Docker" was to make the environment setup instructions for new contributors as streamlined and foolproof as possible. I believe directing them to Docker is the best way to do that. If others disagree, happy to take that conversation elsewhere to not hold up this pull request. Hope that makes sense!

[dsaxton]

Sorry for slow response. My thinking with "splitting out sub-pages for…setting up an environment outside of Docker" was to make the environment setup instructions for new contributors as streamlined and foolproof as possible. I believe directing them to Docker is the best way to do that. If others disagree, happy to take that conversation elsewhere to not hold up this pull request. Hope that makes sense!

Perhaps keep this PR as is and then consider consolidating into a single file for setting up environment later? What do you say @MarcoGorelli?

I do think the idea that Docker is the easiest way to contribute is debatable. Some may see it as a lot of overhead compared to just using a virtual environment.

[MarcoGorelli]

I do think the idea that Docker is the easiest way to contribute is debatable

Yes - also, see this comment by Jeff:

I suppose this is ok, we don't normally recommend using docker at all, but to the extent people use it, having a script is good.

From my own experience of mentoring people at sprints using both Docker and Miniconda, the latter got them up-and-running faster, so I'd rather not keep the Docker instructions separate or make them look like the preferred approach

---

@David-dmh could you fetch and merge upstream/master please?

If you could also make contributing_environment.rst which includes both Docker instructions as well as non-Docker, and remove env_outside_docker, that'd be great, else it can be a follow-up PR

[David-dmh]

I do think the idea that Docker is the easiest way to contribute is debatable

Yes - also, see this comment by Jeff:

I suppose this is ok, we don't normally recommend using docker at all, but to the extent people use it, having a script is good.

From my own experience of mentoring people at sprints using both Docker and Miniconda, the latter got them up-and-running faster, so I'd rather not keep the Docker instructions separate or make them look like the preferred approach

@David-dmh could you fetch and merge upstream/master please?

If you could also make contributing_environment.rst which includes both Docker instructions as well as non-Docker, and remove env_outside_docker, that'd be great, else it can be a follow-up PR

Ok will do so tomorrow

[David-dmh]

Queued now, I just merged.

[David-dmh]

With regards to these 2 errors, are they also unrelated? Both with bash exit code 125.

[dsaxton]

With regards to these 2 errors, are they also unrelated? Both with bash exit code 125.

Very likely unrelated. It looks like we still need to consolidate into contributing_environment.rst like @MarcoGorelli recommended.

[David-dmh]

With regards to these 2 errors, are they also unrelated? Both with bash exit code 125.

Very likely unrelated. It looks like we still need to consolidate into contributing_environment.rst like @MarcoGorelli recommended.

Ok, is the consensus to open a new PR for that or rather to do it here?

[dsaxton]

I suppose creating the environment RST file could be a follow up, otherwise looks good to me

[MarcoGorelli]

That's OK with me, thanks @David-dmh - could you create an issue for creating the environment.rst file it so it doesn't get forgotten please?

I likely won't have time to run this this week, so @dsaxton if it looks good to you don't wait for me to merge

[dsaxton]

It seems like the issue mentioned by @MarcoGorelli in contributing_environment.rst was not resolved (for example it still displays the old Python version). How can this be done? I thought this process would integrate the corresponding changes.

Most likely stale text was copied into the new file when you created it, in which case git has no way of knowing the content should be different. You'll need to apply that change as a new commit.

[David-dmh]

It seems like the issue mentioned by @MarcoGorelli in contributing_environment.rst was not resolved (for example it still displays the old Python version). How can this be done? I thought this process would integrate the corresponding changes.

Most likely stale text was copied into the new file when you created it, in which case git has no way of knowing the content should be different. You'll need to apply that change as a new commit.

Thanks @dsaxton this makes sense. I pushed again to address this issue. If I am correct, it seems all exit codes point to test failures on files I did not modify apart from pulling new changes. Again, thanks for your patience helping me resolve these simple issues I am facing.

[MarcoGorelli]

Thanks for updating - I tried building this but got

/home/marco/pandas-marco/doc/source/getting_started/install.rst:201: WARNING: undefined label: contributing.dev_env 

which seems relevant as you've removed that label

---

Other than that, this looks ready to me!

[David-dmh]

Thanks for updating - I tried building this but got

/home/marco/pandas-marco/doc/source/getting_started/install.rst:201: WARNING: undefined label: contributing.dev_env 

which seems relevant as you've removed that label

Other than that, this looks ready to me!

Ok good, just let me know if there is anything else required to wrap it up.

[MarcoGorelli]

@David-dmh other than resolving that warning, I think this is it - if you ping when it's resolved we'll take another look

[David-dmh]

@David-dmh other than resolving that warning, I think this is it - if you ping when it's resolved we'll take another look

Ok, this last one seems to be due a function definition. I will dig in further to examine in the codebase, do let me know if you have any suggestions.

[MarcoGorelli]

Awesome!

CI failure is unrelated, should be fixed by #40768

@dsaxton @afeld happy for this to be merged or any further comments?

[dsaxton]

@dsaxton @afeld happy for this to be merged or any further comments?

Looks good 👍

[MarcoGorelli]

Thanks @David-dmh !

And thanks @dsaxton for reviewing and @afeld for suggesting the issue!