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

docs: Various improvements to the contributor guide #14151

Closed
wants to merge 6 commits into from

Conversation

BGR360
Copy link
Contributor

@BGR360 BGR360 commented Feb 1, 2024

No description provided.

@github-actions github-actions bot added documentation Improvements or additions to documentation python Related to Python Polars rust Related to Rust Polars labels Feb 1, 2024
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but I won't be merging this. How to configure your virtual environment in VSCode and setting line length markers are outside the scope of what should be part of the Polars contributing guide.

@@ -228,6 +228,7 @@ From the `py-polars` directory, run `make fmt` to make sure your additions pass
Polars uses Sphinx to build the API reference.
This means docstrings in general should follow the [reST](https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html) format.
If you want to build the API reference locally, go to the `py-polars/docs` directory and run `make html SPHINXOPTS=-W`.
By default, this uses only one thread. To speed it up, run `make html SPHINXOPTS="-W -j auto"` to use all available cpus.
Copy link
Member

Choose a reason for hiding this comment

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

This is enabled by default - see the Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran without the -j auto and it went extremely slowly for me and only seemed to be using one core. I added -j auto and it went much faster. Is that just because it had been built once before? Is the first time always really slow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the Makefile sets the following by default, as you said:

SPHINXOPTS ?= -j auto -W

However, if you do as the contrib guide suggests and run make html SPHINXOPTS=-W, then you will override that default and get rid of the -j auto. So we should change the guide to say make html.

Copy link
Member

Choose a reason for hiding this comment

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

You're right!

docs/development/contributing/index.md Show resolved Hide resolved
@stinodego stinodego closed this Feb 1, 2024
@BGR360
Copy link
Contributor Author

BGR360 commented Feb 1, 2024

Hi @stinodego. Can we please not close the entire PR and instead negotiate on edits that you'll accept? You only disagreed with a portion of my edits and then basically threw out the rest. That doesn't make me motivated to continue contributing.

How to configure your virtual environment in VSCode [...] Is out of scope

Then the guide should omit the message saying "ensure that VSCode is configured to use the virtual environment." That line confused me because it is not clear how to configure that particular thing in VSCode. I had to do an unusual amount of googling to figure it out. Either the guide should more clearly explain what that means or it should omit saying it at all IMO.

setting line length markers [...] Is out of scope

Fair. I'm totally fine to remove that section.

The problem I was trying to address is that it wasn't immediately clear to me what the Python line length was in this project. I wasn't familiar with 88 as a standard. If this is already covered in another Code Style page, then I agree there's no sense clarifying it here.

You made no comment on the Ruff changes I made.

@stinodego
Copy link
Member

stinodego commented Feb 6, 2024

You made no comment on the Ruff changes I made.

Again, this is out of scope. The Ruff extension has clear installation instructions, we don't have to copy those.

Can we please not close the entire PR and instead negotiate on edits that you'll accept?

I can understand that it can come off as harsh when your PR is closed. But please understand we have limited time to manage everything. In this case, you just misjudged what type of content we want as part of the contributing guide. If you open PRs that are not part of an accepted issue, you run the risk of missing the mark. That's OK, there are other things you can work on if you're interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants