-
Notifications
You must be signed in to change notification settings - Fork 1
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
[STYLE] Drop python 3.7, linting applied and improvements of it #96
Conversation
"flake8-import-order==0.18.1", | ||
"black==23.3.0", | ||
"flake8==6.0.0", | ||
"isort==5.12.0", |
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.
That's the main part to have a look at, for consistency I used isort and dropped flake8 import order (btw it was not triggered at all)
I did not update mypy because many parts of the code were triggering errors, and they seemed a little bit time consuming to solve, also I think that using pyright instead would be good for consistency with the public APIs.
.pre-commit-config.yaml
Outdated
hooks: | ||
- id: autoflake | ||
- repo: https://github.com/PyCQA/flake8 | ||
rev: 6.0.0 | ||
hooks: | ||
- id: flake8 | ||
exclude: setup.py | ||
language_version: python3.11 | ||
exclude: ^sdk/ |
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.
to prevent non used imports errors being raised
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.
Sounds like this should be handled from the flake8 config instead
@@ -20,7 +20,12 @@ | |||
from pasqal_cloud.batch import Batch, RESULT_POLLING_INTERVAL | |||
from pasqal_cloud.client import Client | |||
from pasqal_cloud.device import BaseConfig, EmulatorType | |||
from pasqal_cloud.endpoints import AUTH0_CONFIG, Auth0Conf, Endpoints, PASQAL_ENDPOINTS | |||
from pasqal_cloud.endpoints import ( # noqa: F401 |
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.
as the endpoints and auth0 config is not used directly here but imported by the users, using the noqa flag prevent flake8 errors
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.
lgtm, two small comments
.pre-commit-config.yaml
Outdated
hooks: | ||
- id: autoflake | ||
- repo: https://github.com/PyCQA/flake8 | ||
rev: 6.0.0 | ||
hooks: | ||
- id: flake8 | ||
exclude: setup.py | ||
language_version: python3.11 | ||
exclude: ^sdk/ |
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.
Sounds like this should be handled from the flake8 config instead
pasqal_cloud/_version.py
Outdated
@@ -12,4 +12,4 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
__version__ = "0.3.3" | |||
__version__ = "0.3.4" |
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.
I don't think this warrants a version bump tbh
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.
Alright, I had a doubt regarding the python version dependency
e5b0b60
to
aa49aec
Compare
This MR is mostly about linting following my previous MR as it wasn't applied correctly. This MR also includes the changes of #88, with some updates of the linting dependencies, the files of the CI and pre commit hooks. There are few things I forgot to change in my last MR, so this version patch them.
The MR is a fork of the following one: #95 (you find some comments from aleksander on it)