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

MNT/STY/TST: Add pre-commit hooks, CI job, standardize style #183

Merged
merged 7 commits into from
Apr 26, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Apr 24, 2024

Description

  • adds pre-commit hooks and relevant .rc files to the repository
  • adds github actions pre-commit job to run on pull requests
  • (WIP) apply changes suggested by pre-commit (flake8, shellcheck, etc)
    • python files
      - [ ] bash scripts (hooooo boy)
  • Converts some old py2 scripts to py3

Motivation and Context

The style is whack.

Shellcheck is a much larger effort, and one that we should be careful with. We'll have to go through those more carefully, but for now the CI will only check diff'd files

I spent way too much time trying to grab specific sha's for pre-commit's --from-ref and --to-ref arguments, but the location of that information in the github payload was inconsistent between push and pull-request. At that point I figured I should just get it working.

How Has This Been Tested?

Pre-commit vomits all over my terminal repeatedly

Where Has This Been Documented?

This PR

@tangkong tangkong force-pushed the mnt_pre_commit branch 2 times, most recently from 74ad6c5 to ee2644a Compare April 25, 2024 16:04
@tangkong tangkong marked this pull request as ready for review April 25, 2024 16:47
@tangkong tangkong requested a review from ZLLentz April 25, 2024 16:50
@tangkong tangkong changed the title WIP: MNT/STY: Add pre-commit hooks, CI job, standardize style MNT/STY/TST: Add pre-commit hooks, CI job, standardize style Apr 25, 2024
if not foundHutch and path.find('xrt')+hostname.find('xrt')>=-1 or path.find('xtod')+hostname.find('xtod')>=-1:
hutch='LFE' #because we have so many names for the same subnet.
foundHutch=True
if (not foundHutch and path.find('xrt')+hostname.find('xrt') >= -1
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly surprised this line passes flake8 with the lack of + spacing

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'm also confused about this...

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I liked the specific implementation of the pre-commit a lot
The specific changes all look correct/nondestructive to me

@tangkong tangkong merged commit cb10fb4 into pcdshub:master Apr 26, 2024
2 checks passed
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