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

Pre-commit and poetry tasks #181

Merged
merged 7 commits into from
Oct 11, 2022
Merged

Pre-commit and poetry tasks #181

merged 7 commits into from
Oct 11, 2022

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Oct 7, 2022

Proposed changes

This PR adds the following:

  • Poetry task to perform testing
  • Poetry task for quality checking
    • This currently includes flake8 and pylint, but we should go through and figure out resolving errors
  • Pre-commit hooks
    • Also includes flake8 and pylint, but for now this is commented out.

I have pointed this PR to poetry_setup rather than master as that should be merged first.

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

  • Changes have been tested to ensure that fix is effective or that a feature works.
  • Changes pass the unit tests
  • Code has been run through black with the -l 79 flag.
  • I have included necessary documentation or comments (as necessary)
  • Any dependent changes have been merged and published

Notes

All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

PR template was adopted from appium

@kaitj kaitj temporarily deployed to TEST October 7, 2022 18:23 Inactive
@kaitj kaitj temporarily deployed to TEST October 7, 2022 18:23 Inactive
@kaitj kaitj temporarily deployed to TEST October 7, 2022 18:23 Inactive
@kaitj kaitj added the enhancement New feature or request label Oct 7, 2022
@kaitj kaitj requested a review from tkkuehn October 7, 2022 18:24
@kaitj kaitj self-assigned this Oct 7, 2022
Base automatically changed from poetry_setup to master October 7, 2022 19:15
Copy link
Collaborator

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, one comment

pyproject.toml Outdated Show resolved Hide resolved
@kaitj kaitj temporarily deployed to TEST October 7, 2022 19:51 Inactive
@kaitj kaitj temporarily deployed to TEST October 7, 2022 19:51 Inactive
@kaitj kaitj temporarily deployed to TEST October 7, 2022 19:51 Inactive
@kaitj
Copy link
Collaborator Author

kaitj commented Oct 7, 2022

Did we want to try to resolve potential warnings as part of this PR or separate (the linting is likely still to fail or output a long list for now).

@tkkuehn
Copy link
Collaborator

tkkuehn commented Oct 7, 2022

Lets fix the rest of them in a separate PR

@tkkuehn
Copy link
Collaborator

tkkuehn commented Oct 7, 2022

Oh, should we use poethepoet in the quality check actions now?

@kaitj
Copy link
Collaborator Author

kaitj commented Oct 7, 2022

This was actually a question I had when I was looking at the snakebids actions since this basically copies that workflow. The action and the poe task differ in one minor way: the poe task actually performs the sorting and corrections but the action only performs checks for isort and black.

Thinking about it some more, if we resolve all the warnings and ensure pre-commit is run, than we can update the poe task to perform the check and use that in the action.

@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:19 Inactive
@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:19 Inactive
@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:19 Inactive
@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:22 Inactive
@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:22 Inactive
@kaitj kaitj temporarily deployed to TEST October 10, 2022 00:22 Inactive
@tkkuehn
Copy link
Collaborator

tkkuehn commented Oct 11, 2022

Yeah I don't think we'll want to perform the corrections in the workflow, so I'm fine either way.

@tkkuehn tkkuehn merged commit 7f72178 into master Oct 11, 2022
@tkkuehn tkkuehn deleted the quality branch October 11, 2022 18:36
@tkkuehn tkkuehn temporarily deployed to TEST October 11, 2022 18:36 Inactive
@tkkuehn tkkuehn temporarily deployed to TEST October 11, 2022 18:36 Inactive
@tkkuehn tkkuehn temporarily deployed to TEST October 11, 2022 18:36 Inactive
@kaitj kaitj added maintenance Pull requests that maintain the repo and removed enhancement New feature or request labels Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Pull requests that maintain the repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants