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

Linting/Refactor for Tests #857

Closed
wants to merge 4 commits into from
Closed

Linting/Refactor for Tests #857

wants to merge 4 commits into from

Conversation

breca
Copy link
Contributor

@breca breca commented Feb 25, 2024

Cleans up current devel branch to satisfy black/pylint (at least for 3.11)

Signed-off-by: Brett Calliss <brett@obligatory.email>
Signed-off-by: Brett Calliss <brett@obligatory.email>
@p12tic
Copy link
Collaborator

p12tic commented Mar 8, 2024

podman compose uses automatic ruff format for enforcing formatting. I think this PR is no longer needed.

@breca
Copy link
Contributor Author

breca commented Mar 8, 2024

Yes, no longer required since project stopped using black and pylint smells are now an internal metric.

What a waste of time!

Closing.

@breca breca closed this Mar 8, 2024
@p12tic
Copy link
Collaborator

p12tic commented Mar 8, 2024

@breca

pylint smells are now an internal metric

All pylint issues have been fixed though and the check has been re-enabled in CI. The project is just using different tool for what both black and pylint did before.

@p12tic
Copy link
Collaborator

p12tic commented Mar 8, 2024

If you think that ruff doesn't cover what pylint covered before then it makes sense to explore how to make ruff behave in appropriate way. Looking into your fixes it seems to be the case...

So I will reopen the PR after all as there is still useful stuff here.

@p12tic p12tic reopened this Mar 8, 2024
breca and others added 2 commits March 9, 2024 20:24
Signed-off-by: Brett Calliss <brett@obligatory.email>
Signed-off-by: Brett Calliss <brett@obligatory.email>
@breca
Copy link
Contributor Author

breca commented Mar 9, 2024

Certainly seem to be some drift between ruff and pylint.

(linting) $ pip list installed | grep lint
pylint          3.0.4

$ python -m pylint podman_compose.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)
(linting) $ git checkout main
Switched to branch 'main'
Your branch is up to date with 'upstream/main'

(main) $ python -m pylint podman_compose.py
************* Module podman_compose
podman_compose.py:1141:22: E1101: Module 'subprocess' has no 'create_subprocess_exec' member (no-member)
podman_compose.py:1146:12: R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it (no-else-return)
podman_compose.py:1163:4: W0102: Dangerous default value set() (builtins.set) as argument (dangerous-default-value)
podman_compose.py:1191:26: E1101: Module 'subprocess' has no 'create_subprocess_exec' member (no-member)
podman_compose.py:1206:26: E1101: Module 'subprocess' has no 'create_subprocess_exec' member (no-member)
podman_compose.py:1894:12: W0719: Raising too general exception: Exception (broad-exception-raised)
podman_compose.py:1976:24: E1101: Module 'subprocess' has no 'create_subprocess_exec' member (no-member)
podman_compose.py:2271:16: W0106: Expression "[_.cancel() for _ in tasks if not _.cancelling() and not _.cancelled()]" is assigned to nothing (expression-not-assigned)

-------------------------------------------------------------------
Your code has been rated at 9.87/10 (previous run: 10.00/10, -0.13)

I've rebased and this now returns a 10/10 again on pylint.

For whatever reason this is now failing on ruff in CI... which I cannot reproduce locally in my virutalenv.

(linting) $ ruff -V
ruff 0.3.1
(linting) $ ruff check
(linting) $ echo $?
0
(linting) $ ruff format --check podman_compose.py 
1 file already formatted

🤷

I'm really not that opinionated about it, some of these findings are false positives anyway.

@baszoetekouw
Copy link
Contributor

I think all of the pylint issues were fixed in #887, which also reenbles pylint in the CI checks.

@p12tic
Copy link
Collaborator

p12tic commented Apr 28, 2024

The current main branch contains everything that was in this PR. I did a local rebase to ensure we don't miss anything. Closing.

@p12tic p12tic closed this Apr 28, 2024
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.

3 participants