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

Setup pyproject.toml for Ruff and MyPy configurations #768

Merged
merged 6 commits into from
May 16, 2024

Conversation

omar-abdelgawad
Copy link
Contributor

@omar-abdelgawad omar-abdelgawad commented May 9, 2024

📢 Describe your changes

  • 📌 changes Summary
    • formatted and type hinted style.py, and presets.py
    • Added pyproject.toml file with configurations for Mypy and Ruff
    • Added Ruff and Mypy to optional-requirements
    • Changed tr variable name (removed)
  • ⚠️ Breaking changes
    • None
  • 🐞 Bug Fixes:
    • None

🧪 How to test this Pull Request

  1. For testing type information using MyPy:
mypy invesalius/presets.py invesalius/style.py --follow-imports=skip --check-untyped-defs \
--disallow-any-generics --disallow-incomplete-defs --warn-redundant-casts --warn-unused-ignores \
--no-implicit-optional --strict-equality --warn-return-any --warn-unreachable --warn-unused-configs 
  1. For testing Ruff linting:
ruff check invesalius/presets.py invesalius/style.py --extend-select UP --extend-select I

🗒️ Notes for future improvements

I use this PR template a lot and I can make a PR to add to the repo if it is good enough. I believe having a template makes documenting pull requests much easier.

✅Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.

formatted and type hinted style.py, and presets.py
pyproject.toml Outdated Show resolved Hide resolved
invesalius/presets.py Outdated Show resolved Hide resolved
@omar-abdelgawad
Copy link
Contributor Author

I remembered that I should add ruff and mypy versions to optional-requirements.txt. I did just that.

@tfmoraes
Copy link
Member

Hi @omar-abdelgawad. I tested here. I'm using Python 3.11. I'm having this error:

  File "/var/home/thiago/Sources/github/invesalius3/invesalius/presets.py", line 33, in <module>
    class Presets:
  File "/var/home/thiago/Sources/github/invesalius3/invesalius/presets.py", line 106, in Presets
    def SavePlist(self, filename: str | Path) -> str:
                                        ^^^^
NameError: name 'Path' is not defined

If I add from __future__ import annotations on the top of invesalius/presets.py it works.

Other thing. I think the tr part is better to be in other PR.

…_ to tr in entire codebase"

This reverts commit b6b5747.
Also reverts some lines in commit 6bead8e where tr was changed.
@omar-abdelgawad
Copy link
Contributor Author

Hi @tfmoraes . I apologize for not noticing this runtime error. I read the docs and evaluating annotations at runtime happens only with function parameters which I missed. I tested it now and it works. I also reverted the changes regarding tr using git revert and then changed it also in presets.py and included this change in the same revert commit using git commit --amend. I thought this would make it more concise but please tell me to fix if it confusing or if you think I should do a complete git rebase -i.

@tfmoraes
Copy link
Member

Thanks @omar-abdelgawad. Merging it.

@tfmoraes tfmoraes merged commit 1db5334 into invesalius:master May 16, 2024
@omar-abdelgawad omar-abdelgawad deleted the type-info branch May 16, 2024 14:34
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.

2 participants