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

Use absolute imports #20

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Use absolute imports #20

merged 5 commits into from
Oct 1, 2024

Conversation

amaloney
Copy link
Member

@amaloney amaloney commented Sep 28, 2024

  • Removes the deprecated absolutify-imports package from .pre-commit and adds a rule in pyproject.toml for ruff to handle relative import errors.
  • Refactors all code that used relative imports.

Resolves #19


📚 Documentation preview 📚: https://arviz-base--20.org.readthedocs.build/en/20/

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
- Removes the deprecated `absolutify-imports` package from `.pre-commit`
  and adds a rule in `pyproject.toml` for `ruff` to handle relative
  import errors.
- Refactors all code that used relative imports.
@amaloney amaloney requested a review from OriolAbril September 28, 2024 21:24
@amaloney amaloney self-assigned this Sep 28, 2024
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

It might also be a good idea to run pre-commit autoupdate now that we are changing some of the config.

pyproject.toml Outdated
Comment on lines 76 to 78
[tool.ruff.lint.flake8-tidy-imports]
ban-relative-imports = "all"

Copy link
Member

Choose a reason for hiding this comment

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

is this different from adding TID252 to the list of selected rules below?

Copy link
Member Author

Choose a reason for hiding this comment

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

This I do not know. I used the documentation to update the pyproject.toml file

https://docs.astral.sh/ruff/settings/#lint_flake8-tidy-imports_ban-relative-imports

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 use both, and move this section below the general tool.ruff.lint this way we'll have all specific configuration of ruff.tool.lint grouped together (there are already pydocstyle and file specific configs). From what I understand, this part is changing the default behaviour of the rule which probably adds it to the list of selected rules under the hood (otherwise configuring it would be pointless) but feels a bit obscure. The docs for the rule: https://docs.astral.sh/ruff/rules/relative-imports/. We can add a comment after the rule explaining what it is like for the others.

@OriolAbril
Copy link
Member

it looks like it now runs on notebooks within docs which fails the check: https://github.com/arviz-devs/arviz-base/actions/runs/11087276115/job/30805831294?pr=20#step:5:37. I think we can skip ruff for the notebooks in the docs

@OriolAbril
Copy link
Member

And nice catch on this: https://github.com/arviz-devs/arviz-base/actions/runs/11087276115/job/30805831294?pr=20#step:5:104 looks like the upgrade is worth it

- also fix test error
@amaloney
Copy link
Member Author

woof...pun intended ruff. At least it is all green now, and thank you @OriolAbril for setting up the CI.

This should refactor the codebase to use absolute imports now.

@OriolAbril OriolAbril merged commit 38aa05e into main Oct 1, 2024
7 checks passed
@OriolAbril OriolAbril deleted the issue19/absolute-imports branch October 1, 2024 20:07
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.

use absolute imports
2 participants