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

MAINT: adding lint.select and corresponding style fixes #67

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Schefflera-Arboricola
Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola commented May 11, 2024

Will open this PR after PR#69 is merged

This PR switches the nx-parallel's building tool from `hatchling` to `setuptools` as it is more widely used and it's better for bigger projects and we need to explicitly mention all the packages(unlike hatchling) so that ensures that all packages are being built.

I didn't add any extra ruff or lint settings. But, the style wf test was failing in github wf so in the last commit in this PR are all the pre-commit changes. I think it is a result of switching from hatchling to setuptools.

moved select, pre-file-ignores under tool.ruff.lint section due to the following warning:

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'select' -> 'lint.select'
  - 'per-file-ignores' -> 'lint.per-file-ignores'

@Schefflera-Arboricola Schefflera-Arboricola marked this pull request as ready for review May 11, 2024 18:37
@dschult
Copy link
Member

dschult commented May 11, 2024

The changes to pyproject.toml look like this better matches networkx. The import changes and package names all look good. Perhaps @jarrodmillman or @MridulS can look at the setuptools settings to make sure they make sense.

Can you add a few sentences to the PR's first post describing what you did? (use the "Edit" option under the 3-dots at the upper right of that first post.) That way reviewers don't have to click somewhere else to figure out what this PR is about.

Just some very basic info (not all has to be there and more obviously can be there): for example:

What?

Why?

How?

Testing?

(example from https://www.pullrequest.com/blog/writing-a-great-pull-request-description/)

For most PRs I look for "What" and "Why" in the description. Jarrod has the release process set up so the title gets added to the release notes (I believe), so that's important too. (As a reviewer I try to remember to update the title before merging to make it helpful for the release notes.)

Copy link
Collaborator

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

+1 for the switch to setuptools, though I think we should probably break out the linting parts of this PR if possible (I suspect it is). Not only will that make this reviewable (right now it's very difficult to distinguish the "real" changes from the automated ones) and it would also allow us to keep the blame clean by ignoring the formatting-only changes!

@Schefflera-Arboricola
Copy link
Member Author

The pyproject.toml file has all the "real" changes and all other files have linting changes. And all the linting modifications are in the order of the import statements. I think this was a result of switching to setuptools because the only new linting specification I added was:

[tool.ruff.format]
docstring-code-format = true

and this is for the styling of docstrings.

it would also allow us to keep the blame clean by ignoring the formatting-only changes!

I think this seems reasonable but idk what would be a quick way to revert these automated changes in this PR. So, I'll make a new PR that changes the pyproject.toml, and then after merging that PR, I'll rebase this PR's branch and then we can merge this. I hope that would be ok. LMK what you think.

@Schefflera-Arboricola Schefflera-Arboricola marked this pull request as draft June 8, 2024 09:36
@Schefflera-Arboricola Schefflera-Arboricola changed the title MAINT: switching to setuptools as the building tool MAINT: switching to setuptools (style fixes) Jun 8, 2024
@Schefflera-Arboricola Schefflera-Arboricola changed the title MAINT: switching to setuptools (style fixes) MAINT: switching to setuptools (2. style fixes) Jun 8, 2024
@Schefflera-Arboricola Schefflera-Arboricola marked this pull request as ready for review June 8, 2024 09:46
@Schefflera-Arboricola Schefflera-Arboricola marked this pull request as draft June 14, 2024 07:48
@Schefflera-Arboricola Schefflera-Arboricola changed the title MAINT: switching to setuptools (2. style fixes) MAINT: adding lint.select and corresponding style fixes Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants