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

add formatting and linting #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add formatting and linting #139

wants to merge 1 commit into from

Conversation

dragonejt
Copy link
Contributor

This PR adds linting using Pylint and formatting using Black, and adds a GitHub Actions workflow to check linting and formatting on each new push or PR. This is functionally optional, but sets a guideline for formatting so there aren't any formatting conflicts or code smells.

Changes

  • Add black. Run the black formatter with black ..
  • Add pylint. Run pylint python linter with pylint <module>.
  • Format code using black
  • Add GitHub Actions workflow that checks linting and formatting on each new push or PR

Testing

  • Unit Tests are succeeding
  • Linting and Formatting checking is working (Pylint is complaining though due to code smells we can fix later)

Copy link
Contributor

@asgibson asgibson left a comment

Choose a reason for hiding this comment

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

This PR is appreciated and the concept is good.

Out of curiosity, do you have rationale behind the choices of applications? Particularly interested in why you chose Black and Pylint versus any other available options.

onair/src/run_scripts/execution_engine.py Show resolved Hide resolved
Comment on lines +8 to +9
- black
- pylint
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to add these as dependencies for all usage? Perhaps we can have a specific environment file just for CI runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true, conda doesn't natively support dev dependencies so I didn't separate them, but I can add something like a environment_dev.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please do so.

.github/workflows/lint.yml Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
@dragonejt
Copy link
Contributor Author

This PR is appreciated and the concept is good.

Out of curiosity, do you have rationale behind the choices of applications? Particularly interested in why you chose Black and Pylint versus any other available options.

I chose Black and Pylint because of their popularity, support, and ruleset. Black and Pylint are probably the more popular and more supported code formatters and linters, and this is helpful when it comes to IDE integrations with VSCode and PyCharm as you can see linter violations directly in your IDE based on the linter rules you have set/unset. Black also contains a superset of the normal PEP 8 python code format standard. There is also just how mature they are, these are relatively mature tools compared to newcomers like Ruff, and that is useful because we don't want our code formatters and linters to break or have breaking changes.

This decision is purely opinion-based, and it is relatively easy to switch to a different code formatter or linter if we want to.

@asgibson
Copy link
Contributor

Thank you for the explanation, it is much appreciated. Once the environment_dev.yml is added and used in CI, I will approve this PR.

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.

2 participants