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

Makefile Linter | Python & Javascript #353

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

BluThaitanium
Copy link
Contributor

Context

Enforce code-style by creating a linting target in the Makefile. Lints Python and Javascript.
Addressed Issue #322

Changes

  • Separate make calls for Python (.py) and Javascript (.js, .jsx, .ts, .tsx)
    ** Called via make lint_python and make lint_javascript
    ** Linting via flake8 and eslint
  • ESLint formatting rules stored in /dashboard/origin-mlx/.eslintrc.yml
  • Formatting changes to all .py files to meet flake8-PEP8 standards
  • Formatting changes to many javascript files
  • Automatic changes to package-lock.json and package.json for ReactJS support for eslint

Notes

  • .eslintrc.yml's overrides parameters store rules that require large changes to the current codebase. These rules are therefore disabled.
  • Linting python files requires a virtual environment stored as venv in the main /mlx/ directory

@BluThaitanium BluThaitanium force-pushed the makefile_linter_python_javascript branch 2 times, most recently from 0c80b67 to 94e47f8 Compare July 7, 2022 03:16
@ckadner ckadner self-requested a review July 7, 2022 14:46
@ckadner
Copy link
Member

ckadner commented Jul 7, 2022

Wow! Thank you Phu. This is a massive change. I might need some more time to properly review :-)

A few quick observations:

  • Many of the Python code style violations are in generated files which we should probably exclude
  • We should probably add some sand-boxing for Python based make targets, using a venv, like you mentioned
  • Did you use autopep8 or some other tool to automatically fix some of the Python code style violations? If so, maybe we can add a script or Make target for future "auto-corrections"

temp_run.sh Outdated Show resolved Hide resolved
@BluThaitanium
Copy link
Contributor Author

Wow! Thank you Phu. This is a massive change. I might need some more time to properly review :-)

A few quick observations:

  • Many of the Python code style violations are in generated files which we should probably exclude
  • We should probably add some sand-boxing for Python based make targets, using a venv, like you mentioned
  • Did you use autopep8 or some other tool to automatically fix some of the Python code style violations? If so, maybe we can add a script or Make target for future "auto-corrections"
  • I'll take a look and see how to exclude those files/directories with flake8
  • I'll see if we could generate a venv for the user if they don't have one
  • Some of the python corrections were fixed with the Black linter. A lot of the other corrections I sifted through manually and fixed, or added # noqa.

@ckadner
Copy link
Member

ckadner commented Jul 8, 2022

Hi @BluThaitanium -- maybe we can add another Make target to actually fix the Python code violations. We could collaborate on that (seeing that I accidentally already committed onto your branch 8-)

  • autopep8 appears to work okay and understands the same codes and uses similar parameters
  • autoflake takes care of unused imports and unused variables

i.e.:

.PHONY: format_python
format_python: venv ## Correct Python code style violations
	@which autoflake > /dev/null || pip install autoflake
	@which autopep8 > /dev/null || pip install autopep8
	@autoflake api tools/python -i --recursive --remove-all-unused-imports \
		--ignore-init-module-imports --remove-unused-variables
	@autopep8 api/ tools/python/ -i --recursive -a -a -a  --experimental \
		--select=E9,E2,E3,E5,F63,F7,F82,F4,F841,W291,W292 \
		--exclude .git,__pycache__,docs/source/conf.py,old,build,dist,venv \
		--max-line-length=140
	@echo "$@: OK"

After that I am down to this manageable number of code style violations:

22    E501 line too long (157 > 140 characters)
10    E999 SyntaxError: invalid syntax
36    F401 'swagger_server.models.api_parameter.ApiParameter' imported but unused
1     F403 'from swagger_server.models import *' used; unable to detect undefined names
1     F405 'ApiAsset' may be undefined, or defined from star imports: swagger_server.models
21    F841 local variable 'dictionary' is assigned to but never used
11    W291 trailing whitespace

With the auto-fix we could also include the generated API code in the linting, and I could add it to the generate_code script later on to not repeatedly have the violations reappear.

@BluThaitanium BluThaitanium force-pushed the makefile_linter_python_javascript branch from 7659afe to 297931d Compare July 20, 2022 00:18
BluThaitanium and others added 11 commits July 19, 2022 20:19
Signed-off-by: BluThaitanium <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
* Update MLX setup instructions for KF 1.5

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
* Also add combine lint target

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: BluThaitanium <[email protected]>
@BluThaitanium BluThaitanium force-pushed the makefile_linter_python_javascript branch from 297931d to 6ab3372 Compare July 20, 2022 00:19
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