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.

Sign up for GitHub

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

Jump to bottom

Pylint enable #5486

Merged
merged 10 commits into from
Feb 28, 2024
Merged

Pylint enable #5486

merged 10 commits into from
Feb 28, 2024

Conversation

adamkankovsky
Copy link
Contributor

Fetch of VladimirSlavik for the right to run the test.
Old PR:#5269

@pep8speaks
Copy link

pep8speaks commented Feb 22, 2024 •
edited
Loading

Hello @adamkankovsky! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 72:100: E501 line too long (100 > 99 characters)
Line 153:100: E501 line too long (108 > 99 characters)

Comment last updated at 2024-02-28 15:32:37 UTC

@github-actions github-actions bot added infrastructure Changes affecting mostly infrastructure f41 labels Feb 22, 2024
@KKoukiou KKoukiou mentioned this pull request Feb 22, 2024
Closed
3 tasks
@github-actions GitHub Actions
Copy link

Infrastructure check failed on these files. Please do a double check of these files before merge!

Makefile.am
anaconda.py
pyanaconda/display.py
pyanaconda/modules/common/task/progress.py
pyanaconda/ui/webui/init.py
tests/Makefile.am
tests/unit_tests/pyanaconda_tests/modules/boss/test_install_category.py

@KKoukiou
Copy link
Contributor

The infra prefix check is just broken for multi commit pRs. Please ignore it.

jkonecny12
jkonecny12 requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thanks a lot for this.

However, could you please take a look on the note above?

tests/unit_tests/pyanaconda_tests/ui/test_webui.py Outdated Show resolved Hide resolved
pyanaconda/modules/subscription/subscription.py Show resolved Hide resolved
@jkonecny12
Copy link
Member

Honestly, I wonder why we have the requirement that all the infra commits have to have the infra: prefix...

@KKoukiou
Copy link
Contributor

Honestly, I wonder why we have the requirement that all the infra commits have to have the infra: prefix...

Thats fine for consistency. The problem is that the check for 'infra' prefix requirements does not check each commit seperately but the PRs file changes as a whole, which is most often wrong.

VladimirSlavik and others added 10 commits February 28, 2024 16:32
@VladimirSlavik @KKoukiou
infra: Get outputs from containerized pylint
8392945 
Invoking the standalone containerized pylint make target now also:
- propagates error code
- provides logs
@KKoukiou
tests: pylint: fix concat string
5f7e9e0
@VladimirSlavik @KKoukiou
pylint: Disable new warnings found by 3.x
9f76ce0
@KKoukiou
maint: fix: W0707(raise-missing-from):pyanaconda/ui/webui/__init__.py…
edce3a6 
…:168,12
@KKoukiou
pyanaconda: task: remove unused variable
cd49098
@KKoukiou
tests: pylint: remove unused false positive
d446ef0
@KKoukiou
pylint: add new false positive
6c1f3c0 
The 'new' method exists on class Bytes,
https://hackage.haskell.org/package/gi-glib-2.0.29/docs/GI-GLib-Structs-Bytes.html#g:method:new
@KKoukiou
tests: pylint: ignore problematic file for astroid
a09cc33 
Needed because of pylint-dev/astroid#2391
@KKoukiou
infra: bump pylint and astroid version
94702d5
@VladimirSlavik @KKoukiou
tests: Run pylint again
bdff5fc
@KKoukiou
Copy link
Contributor

/kickstart-test --waive infra only

@jkonecny12
Copy link
Member

Honestly, I wonder why we have the requirement that all the infra commits have to have the infra: prefix...

Thats fine for consistency. The problem is that the check for 'infra' prefix requirements does not check each commit seperately but the PRs file changes as a whole, which is most often wrong.

I wrote it wrong. I meant why all the commits requires the infra prefix. So yes, exactly what you pointed out.

I think we should change that. We don't want to have red tests.

jkonecny12
jkonecny12 approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jkonecny12
Copy link
Member

Give me a sec I'm testing it locally. Because in the past we had a different result between local run and Actions run.

@jkonecny12
Copy link
Member

After rebuild of the container image, the pylint test was successful for me. So I think you should be fine to merge this.

🎉 🎉 ⭐

@KKoukiou KKoukiou merged commit c01a74e into rhinstaller:master Feb 28, 2024
16 of 17 checks passed
@adamkankovsky adamkankovsky deleted the pylint-enable branch February 28, 2024 21:42
@adamkankovsky adamkankovsky restored the pylint-enable branch March 14, 2024 08:24
@adamkankovsky adamkankovsky deleted the pylint-enable branch March 14, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKoukiou KKoukiou KKoukiou left review comments

@jkonecny12 jkonecny12 jkonecny12 approved these changes

Assignees
No one assigned
Labels
f41 infrastructure Changes affecting mostly infrastructure
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adamkankovsky @pep8speaks @KKoukiou @jkonecny12 @VladimirSlavik