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
Run linter and tests in CI build #48
Run linter and tests in CI build #48
Conversation
@girotobial I've enabled the linter ( Would you like the fixes done in this PR as well? Also are there any other code quality tools that need to be run? |
Before I start adding the fixes I noticed the line length for |
It's for compability with linters like Flake8 or Pylint. They use the PEP-8 standard which limits lines to 79 characters. |
In that case should we add Flake8 or Pylint to the build as well? (And would this be in addition to MyPy, or replacing it?) |
There's one error in the tests:
I could not reproduce this on my Mac with either Python 3.9 (which was the first version that had generics support) or 3.10 - the tests pass locally just fine. |
The curent build uses prospector which bundles pylint and other linters into one install.
I think that is a poetry issue it's saying it's using 3.9 but I suspect it's using 3.8.3 instead. I will try a couple of ideas on my local branch and see if any fix it. |
The idea I would like to try is:
|
Yep, I just realised the answer was just above where I was looking in the log... Poetry is using the runner's default Python 3.8, and not the one installed by setup-python. There's a bug report against the Action about it. |
|
Ok so there is 'a' workaround to this... which is to avoid using generics type hints. Fortunately they were only used in one place in the whole codebase: def greatcircle_distance(
start: tuple[float, float], end: tuple[float, float]
) So while it's not ideal, it may be tolerable to omit their usage here. |
There is bound to be a hole lot more if we continue down the typing route. For now we can use the typing module, but it would still be good to find a fix for the poetry issue, because we are supposed to be using 3.9+. |
src/geo.py
Outdated
def greatcircle_distance( | ||
start: tuple[float, float], end: tuple[float, float] | ||
) -> float: | ||
def greatcircle_distance(start: tuple, end: tuple) -> float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better fix, would be
`from typing import Tuple
def greatcircle_distance(start: Tuple[float, float], end: Tuple[float, float]) -> float:
...
`
We should avoid removing typing information as it will prevent mypy from picking up typing bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'd forgotten about the typing module
.github/workflows/test.yml
Outdated
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Setup Poetry | ||
run: pipx install poetry | ||
- name: Set up Python | ||
uses: actions/setup-python@v3 | ||
uses: actions/setup-python@v4 | ||
with: | ||
cache: poetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe try without cache to see if it runs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas turning off the cache did not work. Still see this:
============================= test session starts ==============================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
...
I had another thought about going the whole nine yards with linting at the moment... Currently this repo is a fork of oscarpilote's version. If this repo is not the authoritative version of OrthoXP, it may be difficult to integrate changes from oscarpilote's version since that does not have linting/style enforcement enabled. It may be that it presents too many difficulties to enforce much beyond The solution would involve
|
Oscar hasn't contributed code to the main repo in a while, the only code pulled over was a fix made by d41k4n on the oscarpilot repo. The organisation for this repo was initially setup after this conversation. I think reversing the fork may be more difficult, we certainly need to get a release out to make this the authoritative version of O4XP. Any thoughts @oscarpilote? |
In the Jenkins project, the process for transferring plugins from 3rd party contributors into the project is based on inverting the fork relationship:
In the case of OrthoXP, step 1 is already done (and possibly step 2) as well. If Oscar isn't available, it's possible to break the fork relationship from our side by contacting GitHub Support. |
We have progress... I've convinced Poetry to use the correct Python version. This means I have been able to reinstate the generics that Python 3.8 didn't like. I also fixed the type stubs for PIL and Requests, which removes the Just in case this all sounds too good to be true, there is a catch... It appears that when you deviate from the system Python on GH Actions, you have to install the native dependencies yourself. (Or at any rate it starts complaining about missing native dependencies when it didn't before.) And to get a recent version of the dependencies that's compatible with our Python bindings, we have to bump the OS to .... |
Well I think we will just have to suck it up and hope that 22.04 comes out of beta soon. Thanks for all the work put into this, I will now merge it. |
Yep that was my thought... all signs pointed towards moving to 22.04, and tolerating the bumps in the road. A couple of points regarding the outstanding MyPy bugs to fix:
As for the rest (problems that MyPy found in Ortho4XP rather than libraries)... I'm not comfortable adding those type hints since (a) the codebase is only just gaining tests at this point and (b) I do not know the code well. If you would be able to do those then that would be great. |
I'll try, I'm still learning the code base bit by bit, I'll have a look through the config options to see if I can get mypy to only trigger on changed code. Looks like it's already picking up some potential bugs though. |
No description provided.