-
Notifications
You must be signed in to change notification settings - Fork 27
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
Revamp schema versioning and dev version trigger #393
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 93.50% 93.54% +0.04%
==========================================
Files 18 18
Lines 2554 2571 +17
Branches 576 579 +3
==========================================
+ Hits 2388 2405 +17
Misses 95 95
Partials 71 71 ☔ View full report in Codecov by Sentry. |
a4480e4
to
f595744
Compare
I added one more thing: make the SCM pretend version thing DRY, and validate it so it stays in sync. |
f595744
to
257b13b
Compare
.github/workflows/tests.yml
Outdated
@@ -9,7 +9,6 @@ on: | |||
# This **must** match the Major.Minor version of the JSON schema file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should remove this comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yes! :)
CI was failing because pydantic 2.9 changes the schema. :( |
run: | | ||
python -m pip install --upgrade pip | ||
pip install -e .[test] | ||
SETUPTOOLS_SCM_PRETEND_VERSION=`cat .SETUPTOOLS_SCM_PRETEND_VERSION` pip install -e .[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(cat .SETUPTOOLS_SCM_PRETEND_VERSION)
may be a bit more portable than using backticks here (though specifying bash is still a good idea so you don't get, say, PowerShell!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to use $(...)
, but I decided backticks were a better choice. Now I don't remember why...
Does $(...)
work with POSIX Bourne shell? I might have hesitated because of not being sure that would be the case, but maybe that's misguided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no, that was misguided, https://stackoverflow.com/questions/22709371/backticks-vs-braces-in-bash says $()
is POSIX compliant, and backticks are deprecated! I learned something today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the nice thing about $()
is that it nests, e.g.
echo $(echo $(echo foo) $(echo bar))
print( | ||
dedent( | ||
f""" | ||
Schema {schema_path} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these line lengths are rather long!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted a bit, but they're sized to look decent after dedenting, so they're going to stay somewhat long.
I agree they were too long, though, so I adjusted them somewhat.
def test_update_schema(self): | ||
# It's an error for the currently saved schema to be out of date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just assume all of this works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works too well, killed CI when it ran with Pydantic 2.9.1... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I am not very awake, I clickd "add single comment" when I meant to click "start a review. Sorry about that. This all looks good to me, assuming it passes tests and coverage.
92b037c
to
c75ffa9
Compare
squashed with test: exercise the modified g2p update-schema logic
We had forgotten to bump the heroku pretend version from 2.0 to 2.1. This is going to happen again and again, so let's put a stop to it all now! The new file .SETUPTOOLS_SCM_PRETEND_VERSION has the pretend version we'll use anywhere we have to do these fake mechanics, with the current major.minor, and we'll make it an error in unit testing if it's out of sync. When we start working a the next major or minor version, we'll need that dev tag for all this to work, which is good because this way we won't forget! And if we try to put the new major or minor version and didn't do it, it'll fail to run the tests.
c75ffa9
to
208a8e0
Compare
PR Goal?
Clean up how schemas a generated and unit tested
Fixes?
Fixes #392
Also fixes the chicken and egg problem when we start working on the next minor: since the
setuptools_scm
uses tags to determine the current version, you'd have to tag 2.2.0 if the current is 2.1.0 before it would install with the right version, but that's not right since 2.2.0 would not have been published yet. But, in fact, the solution is simple:Feedback sought?
general validation
@roedoejet make sure the schema logic is good
@dhdaines confirm my interpretation of dev tagging with dynamic versioning
Priority?
low
Tests added?
yup, loads!
How to test?
Run
g2p update-schema
with the current schema being up to date, wrong, a new version without change, a new version with change (unit testing covers all these cases).Confidence?
moderate
Version change?
No, but will make the next change cleaner.