Clean up pylint command #5014
Clean up pylint command #5014
Conversation
Signed-off-by: harupy <hkawamura0130@gmail.com>
| msg_template="{path} ({line},{column}): [{msg_id} {symbol}] {msg}" | ||
|
|
||
| pylint --jobs=0 --msg-template="$msg_template" --rcfile="$FWDIR/pylintrc" $(git ls-files | grep $include | grep -v $exclude) | ||
| pylint $(git ls-files | grep '\.py$') |
There was a problem hiding this comment.
Once this PR has been merged, we can run pylint $(git diff --name-only | grep '\.py$') before making a commit to make sure there is no pylint error.
There was a problem hiding this comment.
You can also run pylint $(git diff --name-only | grep '\.py$') using a pre-commit for Git (this is the biggest motication of this PR for me).
| include="^\($(join "\|" "${include_dirs[@]}")\)/.\+\.py$" | ||
| msg_template="{path} ({line},{column}): [{msg_id} {symbol}] {msg}" | ||
|
|
||
| pylint --jobs=0 --msg-template="$msg_template" --rcfile="$FWDIR/pylintrc" $(git ls-files | grep $include | grep -v $exclude) |
There was a problem hiding this comment.
I made the following changes:
- Moved
--jobsis specified inpylintrc. - Removed
--msg-templatesince the default template is good enough. - Removed
--rcfilebecausepylintautomatically detectspylintrc.
| # Add files or directories to the denylist. They should be base names, not | ||
| # paths. | ||
| ignore=build,protos,sdk,db_migrations,temporary_db_migrations_for_pre_1_users | ||
|
|
||
| ignore= |
There was a problem hiding this comment.
Switched to using ignore-paths which is more accurate because it matches against paths, not base names.
| else: | ||
| return [] | ||
| return [] |
There was a problem hiding this comment.
Required this change because pylint now checks scripts in the dev directory.
There was a problem hiding this comment.
So pylint print warning for for ... else usage ?
There was a problem hiding this comment.
@WeichenXu123 No, pylint raises a warning when we use for-else without a break statement in the loop:
# Example
def contains_magic_number(list, magic_number):
for i in list:
if i == magic_number:
print("This list contains the magic number")
else:
print("This list does NOT contain the magic number")
| res = subprocess.run(["pip", "list"], stdout=subprocess.PIPE) | ||
| res = subprocess.run(["pip", "list"], stdout=subprocess.PIPE, check=True) |
There was a problem hiding this comment.
Fixed this for the same reason as https://github.com/mlflow/mlflow/pull/5014/files#r744341750.
Signed-off-by: harupy hkawamura0130@gmail.com
What changes are proposed in this pull request?
Clean up pylint command by specifying required options in
pylintrc.How is this patch tested?
NA
Release Notes
Is this a user-facing change?
(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts: Artifact stores and artifact loggingarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesarea/examples: Example codearea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/projects: MLproject format, project running backendsarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra: MLflow Tracking server backendarea/tracking: Tracking Service, tracking client APIs, autologgingInterface
area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows: Windows supportLanguage
language/r: R APIs and clientslanguage/java: Java APIs and clientslanguage/new: Proposals for new client languagesIntegrations
integrations/azure: Azure and Azure ML integrationsintegrations/sagemaker: SageMaker integrationsintegrations/databricks: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes