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

Feature/add linting #18

Closed
wants to merge 8 commits into from
Closed

Feature/add linting #18

wants to merge 8 commits into from

Conversation

Guille-Barrena
Copy link
Contributor

@Guille-Barrena Guille-Barrena commented May 24, 2023

Add Linting to the project

Linting has been added to improve the development experience.

Important

Testing for Python 3.7 has been removed as official support ends on June 2023
Testing for Python 3.9 has been added

- Refactor Dockerfile
- Refactor makefile
- Update pipfile
- Update pipfile.lock
- create .flake8 file
- create pyproject.toml
- update test.yml from github actions
- remove python 3.7 support
- add python 3.9 support
- Update github actions
@Guille-Barrena Guille-Barrena self-assigned this May 24, 2023
@Guille-Barrena Guille-Barrena requested a review from a team as a code owner May 24, 2023 21:18
@@ -7,7 +7,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [ '3.7', '3.8' ]
python-version: [ '3.8', '3.9' ]
Copy link
Contributor

@lpl lpl May 25, 2023

Choose a reason for hiding this comment

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

I'd add 3.10 and 3.11 if no adjustments are needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Pipfile Outdated
Comment on lines 13 to 15
black = "==23.1.0"
flake8 = "==6.0.0"
mypy = "==1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpl more context about this please

pyproject.toml Outdated
'''

[tool.mypy]
ignore_missing_imports = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dangerous, it won't follow imports when scanning if it cannot find it. In the past (and I think that also currently in some other projects) we had serious troubles with typing correctness because we initially set this to true.

I suggest you to remove this line and check if we can easily (30m max) fix the typing problems that appear. If it's too much effort, let's add a comment for the future reader showing that this line should be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will check.

Note: this lines are also in other projects, we may also investigate it

- Add python version on github actions
- Move from pipenv to poetry
- Remove unused_imports from mypy options
- Upgrade 3.8
- Add poetry lock
# Conflicts:
#	setup.py
#	src/pypendency/exceptions.py
- Lint fixes
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.

None yet

3 participants