Pin black and ruff versions

thabbott commented 10 months ago

Closes #145

Changes

Modify pyproject.toml to pin black version (currently 24.1.1) and ruff version (currently 0.1.15), and add a black configuration argument so that black errors if it doesn't match the pinned version when run. AFAIK this is the only file that needs modification.

The changes don't quite meet the goal of avoiding duplicate version declarations (we're limited by the TOML file spec), but a mismatch between the black version pinned in development dependencies and the required version specified in tools.black should show up early in tests.

ruff doesn't have a flag that checks for a required version, so these changes won't prevent somebody from using a manually-up/downgraded version.

Internals

Pin black and ruff versions for consistency between local and CI/CD environments

Tests

[x] QC test passes locally (make test)
[ ] CI tests pass

Reviewer

@trdean1

trdean1 commented 10 months ago

Awesome, yes I think this should do it!

Maybe a question for @zebengberg - is there a reason why the check in the action is different than the check in the makefile when it comes to what black gets run over?

I think this is why Tristian had to update the two test files here, but those did not get flagged by the CI/CD tests when I made my changes

zebengberg commented 10 months ago

Awesome, yes I think this should do it!

Maybe a question for @zebengberg - is there a reason why the check in the action is different than the check in the makefile when it comes to what black gets run over?

I think this is why Tristian had to update the two test files here, but those did not get flagged by the CI/CD tests when I made my changes

I think it's an historical artifact. We should try to rectify this.

I'm not sure if we can run Makefile recipes in the windows workflows, so addressing this may not be as simple as copying the recipe.

mlshapiro commented 10 months ago

Awesome, yes I think this should do it! Maybe a question for @zebengberg - is there a reason why the check in the action is different than the check in the makefile when it comes to what black gets run over? I think this is why Tristian had to update the two test files here, but those did not get flagged by the CI/CD tests when I made my changes

I think it's an historical artifact. We should try to rectify this.

I'm not sure if we can run Makefile recipes in the windows workflows, so addressing this may not be as simple as copying the recipe.

~~I can attest that this is true. Unfortunately we can't run the make on windows, so we'll either have to run the commands in CI, or just run the commands on the windows specific install.~~

I take this back - we should try to install make using chocolatey.org on the Windows runner (choco install make). This may be frustrating, but i'm sure someone has done it out there. Then we can use make recipes on the windows runner.

Havin the windows specific test is important, so its worth the effort.

@thabbott Feel free to go ahead with this in this in the PR

thabbott commented 10 months ago

Awesome, yes I think this should do it! Maybe a question for @zebengberg - is there a reason why the check in the action is different than the check in the makefile when it comes to what black gets run over? I think this is why Tristian had to update the two test files here, but those did not get flagged by the CI/CD tests when I made my changes

I think it's an historical artifact. We should try to rectify this.

I'm not sure if we can run Makefile recipes in the windows workflows, so addressing this may not be as simple as copying the recipe.

I'm a little confused what this has to do with Makefile recipes in Windows workflows. Is the idea that we'd like to be able to run make black-check in the GitHub action (so we don't have to manually maintain agreement with the Makefile), but can't because we don't currently install make in Windows workflows?

thabbott commented 9 months ago

I think I've taken care of the windows workflow issues: test.yaml now

includes a step to install make on windows runners
uses Makefile recipes to installs a pycontrails dev environment and lint/test
specifies bash as the shell for the lint/test step, which stops windows workflows from passing despite linting errors (compare https://github.com/contrailcirrus/pycontrails/pull/146/commits/6dc660d679f77dc9233e4a177cf513318ad1972a, where windows runners should fail but don't, to https://github.com/contrailcirrus/pycontrails/pull/146/commits/dc06df164dd3597ed09acb9a054484512b4dd156, where they correctly fail)

Any outstanding issues I should address before I merge? (Can I use a Makefile recipe to install pycontrails-bada, or is should I leave those steps as they are?)

zebengberg commented 9 months ago

Can I use a Makefile recipe to install pycontrails-bada, or is should I leave those steps as they are?

I think leave as is! I don't think this recipe would work for windows, even with your support for make here.

contrailcirrus / pycontrails