-
Notifications
You must be signed in to change notification settings - Fork 32
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
requirements: Add setuptools #126
Conversation
the setuptools are no longer defaults and need to be stated explicitly. Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
@pevogam could you please review this simple CI fix? |
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 have some unanswered questions from the intent of the commit messages but other than that thanks for restoring the CI here, a very vital for the health of the project.
@@ -503,6 +503,7 @@ def find_unused_port(session, start_port=10024): | |||
:returns: unused port that was found | |||
:rtype: int | |||
""" | |||
# pylint: disable=C0301 |
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 wonder here (and the commit message adding these lines doesn't answer it) why are these changes now? Is it because the CI didn't run for a while? At first I thought it is related to bumping some new version of linter but pydocstyle doesn't seem related here. Even if it was an offline CI (we could clarify this in the commit message) I still don't quite understand how these changes would pass undetected when the pull request was merged (I think the CI worked fine there).
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.
We are using the latest python3, which happens to be 3.12 now and it does not work with the old bumped pylint. So yes,I had to update the pylint and related modules and these seem to complain about this line (although it should have been probably detected earlier)
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.
Ok, then let's prioritize restoring the CI. On my side I believe it is useful to investigate such external changes and document them in commit messages since pylint is not supposed to change its requirements on pre-existing and already linted issues like C0301.
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.
It shouldn't be a new issue, it's just issue that wasn't caught by previous version (probably bugfix on pylint side). But yeah, I could have improved the commit message but as it was just a style fix.
Thank you, Plamen, for reviewing this, let me merge it so the CI works well again. |
the setuptools are no longer defaults and need to be stated explicitly.