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

feat: including generator meta tag in .readalong files #226

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

roedoejet
Copy link
Collaborator

@roedoejet roedoejet commented Jun 26, 2024

Rebased duplicate of #225 - please either close this one or close #225 and re-request the reviews here.

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.24%. Comparing base (4a333bf) to head (2cea196).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
+ Coverage   87.22%   87.24%   +0.02%     
==========================================
  Files          21       21              
  Lines        1769     1772       +3     
  Branches      321      321              
==========================================
+ Hits         1543     1546       +3     
  Misses        189      189              
  Partials       37       37              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@deltork deltork left a comment

Choose a reason for hiding this comment

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

ready for merge

@deltork deltork requested a review from joanise July 8, 2024 16:21
@joanise
Copy link
Member

joanise commented Jul 10, 2024

Just discovered an issue while discussing #229, repeating the comment here since it's relevant here:

Oh, wait, no, but that uncovers a bug in versioning-patch: VERSION and __version__ were previously meant the be tied, with VERSION being Major.minor and __version__ being generated as Major.minor.patch. On the other hand, the format_version introduced here will need a new variable, not defined before, because we cannot assume it's always going to be the same as readalongs' Major.minor version: we might add features or make breaking changes that don't affect for the .readalong format, in which case we'll be the software minor and/or major, but we won't change the format version unless that too gets modified.

@joanise
Copy link
Member

joanise commented Jul 10, 2024

Also, cli-guide.md has a reference to read-along-1.0.dtd which needs to get bumped to 1.1.

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Please create a new constant, as mentioned above, for format_version.

deltork and others added 2 commits July 12, 2024 10:45
test: fixing ci

conflict resolution: VERSION is now only in readalongs/_version.py
TODO: __version__ is no longer defined, and we need a FORMAT_VERSION
@joanise
Copy link
Member

joanise commented Jul 12, 2024

Since I broke this code with my other PR, I rebased it for you and created the new variable. @deltork take a look and make sure it's OK. I'll approve, but it's also worth a re-review by someone else. @roedoejet ?



if __name__ == "__main__":
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

This file was in CRLF so I fixed that, hence this apparent whole file diff. Hide whitespace changes to view the real diff.

@deltork
Copy link
Collaborator

deltork commented Jul 12, 2024

Also, cli-guide.md has a reference to read-along-1.0.dtd which needs to get bumped to 1.1.

DONE

@deltork
Copy link
Collaborator

deltork commented Jul 12, 2024

Please create a new constant, as mentioned above, for format_version.

Thanks for doing this

Since I broke this code with my other PR, I rebased it for you and created the new variable. @deltork take a look and make sure it's OK. I'll approve, but it's also worth a re-review by someone else. @roedoejet ?

Looks good!

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

My requested changes are done.

@joanise joanise merged commit 29ec752 into main Jul 12, 2024
6 checks passed
@joanise joanise deleted the dev.ap/versioning-patch branch July 12, 2024 17:50
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.

3 participants