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

Black formatting and workflow integrations #475

Merged
merged 21 commits into from
May 8, 2024
Merged

Conversation

RemingtonRohel
Copy link
Contributor

Added pre-commit and GitHub workflow integrations for enforcing code standards.

  • pre-commit has the following hooks:
    • enforces newline at end of file
    • trims trailing whitespace
    • sorts requirements.txt files
    • checks validity of toml and yaml files
    • reformats code with Black
    • checks for single-backtick usage in .rst files
    • checks for poorly formatted directives in .rst files (double colons)
    • checks for improperly formatted inline code in .rst files
    • removes unused imports in Python files
    • reformats C++ code according to Google style guide
    • runs static analysis of C++ code
  • Added two GitHub workflows:
    • Check Python code formatting for Black compatibility on push and pull request
    • Add links to ReadTheDocs build on each PR
  • Almost every file has been reformatted by these integrations. To preserve the git blame, the two commits that reformatted Python and C++ files respectively have been added to a .git-blame-ignore-revs file which Git and GitHub respect.
  • Added page to documentation about contributing to Borealis.

Most of the dependencies for using pre-commit on a machine can be installed in a virtual environment from the pyproject.toml specification for Borealis; within the Borealis directory, run pip install .[dev]. You will also need to install cppcheck, which can be installed via zypper (or your native package manager).

* pre-commit uses some general file hooks, black hooks, and rst hooks
* black configured for github workflows to run on push and pull request
* autoflake for removing unused imports in Python files
* clang-format for formatting C++ code
* cppcheck for static analysis of C++ code
* check-toml and check-yaml for parsing those filetypes
* requirements-txt-fixer for sorting requirements.txt files
* check-hooks-apply and check-useless-excludes to verify that pre-commit hooks and excludes are applicable
* Added Black badge to README.md
@RemingtonRohel RemingtonRohel marked this pull request as draft May 3, 2024 18:32
@RemingtonRohel RemingtonRohel marked this pull request as ready for review May 3, 2024 18:41
@RemingtonRohel RemingtonRohel marked this pull request as draft May 3, 2024 19:57
@RemingtonRohel RemingtonRohel marked this pull request as ready for review May 3, 2024 20:13
@RemingtonRohel RemingtonRohel self-assigned this May 3, 2024
@RemingtonRohel RemingtonRohel linked an issue May 3, 2024 that may be closed by this pull request
Copy link
Contributor

@tjk584 tjk584 left a comment

Choose a reason for hiding this comment

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

I like the overall consistency - 90% of it looks great. There are few instances where the linter changes commands to use multiple lines, making them less readable. If we decide this is our standard, I guess its alright - we'd be consistent at least.

docs/conf.py Show resolved Hide resolved
docs/source/development.rst Show resolved Hide resolved
scheduler/local_scd_server.py Show resolved Hide resolved
scheduler/local_scd_server.py Outdated Show resolved Hide resolved
scripts/steamed_hams.py Outdated Show resolved Hide resolved
scripts/steamed_hams.py Outdated Show resolved Hide resolved
scripts/steamed_hams.py Outdated Show resolved Hide resolved
@RemingtonRohel
Copy link
Contributor Author

See https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html for the rules that Black adheres to when reformatting code

@RemingtonRohel RemingtonRohel requested a review from tjk584 May 6, 2024 20:56
Copy link
Contributor

@Doreban Doreban left a comment

Choose a reason for hiding this comment

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

Looks great! Overall I'm a big fan of the formatting changes. Make sure you have tested to make sure everything works properly though. Run the radar in both release and debug mode, test the scheduler, test the experiments, run any other test scripts we have that are functional, etc...

As long as everything still works properly, I think this will be a very welcome update to the code.

Comment on lines +150 to +159
args = [
"--site_id",
self.site_id,
"--experiments",
line_dict["experiment"],
"--kwargs",
line_dict["kwargs"],
"--module",
"experiment_unittests",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is a bit rough to be split. I'm not sure my suggestion here is doable with black formatting. Maybe if this was changed to a dictionary? It would just be nice to have the --option on the same line as the option value.

Suggested change
args = [
"--site_id",
self.site_id,
"--experiments",
line_dict["experiment"],
"--kwargs",
line_dict["kwargs"],
"--module",
"experiment_unittests",
]
args = [
"--site_id", self.site_id,
"--experiments", line_dict["experiment"],
"--kwargs", line_dict["kwargs"],
"--module", "experiment_unittests",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but that's just the way it works, your suggestion does not pass the pre-commit Black formatting check. This a feature of Black; it has an opinion about how things should be formatted, and it is rigid in enforcing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to this file? I think it's fine, a whole section seems to have been removed so I just want to make sure someone double checks it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this file contained a bunch of results from having run the notebook, which I have removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I.e. the reformatting didn't touch this file, I did

@RemingtonRohel
Copy link
Contributor Author

I've tested running normalscan on the lab computer in release and engineeringdebug modes. All unit testing modules (scheduler, config file, experiments, and the realtime simulator) run as expected (with the caveat that the borealis_experiments repo should be on branch config_antennas. Need to merge that to develop and update the submodule commit before merging).

@RemingtonRohel RemingtonRohel requested a review from tjk584 May 8, 2024 20:22
@RemingtonRohel RemingtonRohel merged commit eac4f69 into develop May 8, 2024
3 checks passed
@RemingtonRohel RemingtonRohel deleted the black_formatting branch May 8, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent Python formatting with Black
3 participants