-
Notifications
You must be signed in to change notification settings - Fork 48
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
[AT-4082] Configure pylint for catalog #549
Conversation
@@ -30,6 +30,12 @@ You can check your current version with the following command: | |||
|
|||
For more information, see [UP42 Python package description](https://pypi.org/project/up42-py/). | |||
|
|||
## 0.37.0a0 |
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.
check the order of merging as the other PR has the same version bump
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'll address that in my PR if this is merged 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.
I updated it to 0.37.0a1
@@ -1,19 +1,18 @@ | |||
import json | |||
import os | |||
import pathlib |
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.
Just for curiosity, Why did we make this change?
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.
Google style code
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.
Line 52 in e0e0e13
pylint-google-style-guide-imports-enforcing = "^1.3.0" |
I check on the surface. Although I found it OK, I will leave the approval for @javidq.
It LGTM. good job @seedlit , this one is a massive change. :) |
pylintrc
Outdated
max-line-length=120 | ||
|
||
# Maximum number of characters on a single line. | ||
max-line-length=150 |
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 actually need to decrease, not increase
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.
@seedlit this suggestion led to explosive growth of the PR and also security gate now fails due to coverage, let's just keep it 120 as it was and let's add a ticket to address it later separately. Sorry for confusion and unneeded work.
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 will create a ticket.
Although I find it surprising that coverage got reduced just because of this setting.
Doesn't makes much sense 🤔
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.
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 reduced because it basically checked all files, so now it shows the coverage of the whole SDK
@@ -1,19 +1,18 @@ | |||
import json | |||
import os | |||
import pathlib |
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.
Google style code
@@ -30,6 +30,12 @@ You can check your current version with the following command: | |||
|
|||
For more information, see [UP42 Python package description](https://pypi.org/project/up42-py/). | |||
|
|||
## 0.37.0a0 |
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'll address that in my PR if this is merged earlier
Quality Gate passedIssues Measures |
@dustgalactic I pushed your suggested changes. |
Jira ticket here.
Scope:
NOTES:
Items:
For release: