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

Linting for shell and GitHub Actions #206

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Linting for shell and GitHub Actions #206

merged 10 commits into from
Dec 18, 2024

Conversation

jwallwork23
Copy link
Contributor

More linting!

Adds linting for shell and GitHub Actions workflows:

Also fixes an oversight in the CMake linting setup.

@jwallwork23 jwallwork23 added the testing Related to FTorch testing label Dec 16, 2024
@jwallwork23 jwallwork23 self-assigned this Dec 16, 2024
@jwallwork23
Copy link
Contributor Author

My initial push at commit e128fe3 shows the static analysis workflow failing because I'd only applied lint fixes to the static_analysis.yml. With my second push at commit 5858a6b it passes.

@jwallwork23 jwallwork23 mentioned this pull request Dec 16, 2024
6 tasks
Previously I hard-coded the path to libtorch. This commit replaces the
hard-code path using `pip show` to get the updated location.
@jwallwork23 jwallwork23 marked this pull request as ready for review December 18, 2024 09:48
@jwallwork23
Copy link
Contributor Author

#207 has been merged into the branch here so should be merged first.

@jwallwork23 jwallwork23 requested review from TomMelt, jatkinson1000 and ma595 and removed request for TomMelt December 18, 2024 09:49
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

They seem active projects.
I note zizmor is in beta, so perhaps we want to pin the version similar to fortitude, to avoid unexpected complaints.

We also need the docs updating to detail these, and I note that shellcheck is not pip installable. Seems installable via all sensible package managers. I wouldn't know where to start with Windows packaga managers, but don't see this as an issue as our docs state "follow the instructions in the linter docs".

@jwallwork23
Copy link
Contributor Author

They seem active projects. I note zizmor is in beta, so perhaps we want to pin the version similar to fortitude, to avoid unexpected complaints.

We also need the docs updating to detail these, and I note that shellcheck is not pip installable. Seems installable via all sensible package managers. I wouldn't know where to start with Windows packaga managers, but don't see this as an issue as our docs state "follow the instructions in the linter docs".

Thanks @jatkinson1000. Addressed in 2834064.

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Thanks @jwallwork23 All LGTM now!

@jwallwork23 jwallwork23 merged commit 11dcd03 into main Dec 18, 2024
6 checks passed
@jwallwork23 jwallwork23 deleted the shellcheck branch December 18, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to FTorch testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants