Skip to content

Conversation

gavindsouza
Copy link
Contributor

@gavindsouza gavindsouza commented Sep 14, 2023

Changes

  • Merge GitHub Actions manifests into one for Lint & Unit Tests jobs
  • Update supported env list to [3.8, 3.9, 3.10, 3.11]
  • Add Pre-commit File - Run pre-commit install to setup your hooks

Fixes #748, #747

@gavindsouza gavindsouza requested a review from a team as a code owner September 14, 2023 13:24
@AzisK
Copy link
Contributor

AzisK commented Sep 14, 2023

Good job Gavin. Could you also update the checkout Github Action actions/checkout@v1 since there is already v4 out there?

Nevermind, I see you bumped it already

@coveralls
Copy link

coveralls commented Sep 14, 2023

Coverage Status

coverage: 98.423% (+0.002%) from 98.421% when pulling 25a0484 on gavindsouza:refactor-ci into 30574f9 on kayak:master.

@gavindsouza
Copy link
Contributor Author

Hey @AzisK, is there anything else I can do to help get this merged?

@AzisK
Copy link
Contributor

AzisK commented Sep 20, 2023

@gavindsouza it looks good but before that could you look at my last comments about pypy and black target version and tox envlist?

@gavindsouza
Copy link
Contributor Author

@gavindsouza it looks good but before that could you look at my last comments about pypy and black target version and tox envlist?

I can't find anything regarding those yet to be addressed. (Did you add comments that are in a Pending state? If so, you are yet to submit your review)


Meanwhile, since black is moved to pre-commit, I think removing it from dev-requirements tracking is best.

tox.ini Outdated
@@ -1,13 +1,13 @@
[tox]
envlist = py36,py37,py38,py39,pypy3
envlist = py38,py39,py310,py311,py312,pypy3
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove version 12 from here as well if it is unstable with Github Action actions/checkout@v4?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also wondering why do we have pypy3 here if we are not running any tests with it. Is it easy to setup a test for it, too?

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've removed 3.12. About the pypy3 in the envlist, I'm not sure - I guess someone was using it in their development flows however I don't have a requirement on pypy so I haven't explored 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.

I think it's best to leave pypy3 as is. It doesn't change anything for people who don't use it. If you care about it and have pypy3 setup on your local machine, it just works for you

@AzisK
Copy link
Contributor

AzisK commented Sep 21, 2023

@gavindsouza

RE: Meanwhile, since black is moved to pre-commit, I think removing it from dev-requirements tracking is best.

I agree

RE: I can't find anything regarding those yet to be addressed. (Did you add comments that are in a Pending state? If so, you are yet to submit your review)

Yes, they have been in a pending state, sorry about that and thanks for the notice

@AzisK
Copy link
Contributor

AzisK commented Sep 21, 2023

Everything looks good, thanks! I will merge the pull request but version bump and release will happen a little bit later. However, this mostly concerns the CI, so I believe the version is not even very important in this case

@AzisK AzisK merged commit a1b0c82 into kayak:master Sep 21, 2023
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.

Add pre-commit hook to lint the code
3 participants