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

[SYNPY-1274] pre commit in gh action #1001

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

BryanFauble
Copy link
Contributor

Problem:
We were only running the black linter during the GH action - which does not enforce everything the pre-commit check did.

Solution:
I set up pre-commit job that runs this action: https://github.com/pre-commit/action

Testing:

  1. Failure in pre-commit: https://github.com/Sage-Bionetworks/synapsePythonClient/actions/runs/6710081386/job/18234512611
  2. Success in pre-commit: https://github.com/Sage-Bionetworks/synapsePythonClient/actions/runs/6710112140/job/18234608303

@BryanFauble BryanFauble requested a review from a team as a code owner October 31, 2023 17:19
@@ -1,4 +1,3 @@
sphinx
sphinx-argparse
sphinx_rtd_theme
-e .
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having this included was giving me:

ERROR: file:///home/bfauble/BryansGreatWorkspace/synapsePythonClient/docs (from -r requirements.txt (line 4)) does not appear to be a Python project: neither 'setup.py' nor 'pyproject.toml' found.

Also - the pre-commit wanted to move this to line 1 - which is what prompted me to look at this.

Copy link
Member

Choose a reason for hiding this comment

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

I think this was included only for a test read the docs integration.

Let's revisit the doc deployment strategy later on. It's an inconvenience that we have to build it ourselves when deploying - and our build doesn't allow for deployment of docs of past versions on our site.

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 Going to pre-approve.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -1,4 +1,3 @@
sphinx
sphinx-argparse
sphinx_rtd_theme
-e .
Copy link
Member

Choose a reason for hiding this comment

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

I think this was included only for a test read the docs integration.

Let's revisit the doc deployment strategy later on. It's an inconvenience that we have to build it ourselves when deploying - and our build doesn't allow for deployment of docs of past versions on our site.

@BryanFauble BryanFauble merged commit 9218134 into develop Oct 31, 2023
35 checks passed
BryanFauble added a commit that referenced this pull request Nov 1, 2023
* run pre-commit before test stage
@BryanFauble BryanFauble deleted the SYNPY-1274-pre-commit-in-gh-action branch November 1, 2023 16:06
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.

None yet

2 participants