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

Add version.py template #39

Closed
wants to merge 1 commit into from
Closed

Add version.py template #39

wants to merge 1 commit into from

Conversation

tbekolay
Copy link
Member

@tbekolay tbekolay commented May 31, 2019

Motivation and context:
Similar to #38, realized I was going to copy/paste something in, so opted for Nengo Bones instead.

In the case of a project's version number, there are actually several other places where we end up using this information, (usually files that we're already templating) so by adding version information to the top-level config dictionary we can simplify several places that used to import the version.py module.

However, that's created a bit of spiral in that now we have to add version information to all our tests and docs. I did it for the tests, but before I work on this further, I wanted to get some feedback on this PR.

One thing is that I opted for the user to fully specify the semantic version like so:

version:
  major: 3
  minor: 2
  patch: 1
  release: false

This is the most verbose and (perhaps?) least error-prone way to edit the file. I thought it would be easiest for the underlying implementation too, but it turns out we use the full version string ("v3.2.1.dev0" in the case above) several times, so I kind of hacked it an auto-generated version.full key in as a convenience.

Another option would instead be to do something like

version: 3.2.1.dev0

This is more flexible, but (possibly?) more error prone to edit, especially since you have to know that the .dev0 string is what differentiates a release version from a non-release version. The reason why I didn't do this initially is that I didn't want to copy some annoying code to split up this string into its component parts, but that was before I auto-generated version.full; we could do the same auto-generating to isolate the major, minor, patch, release as well.

It should be noted that this is the only real place where we do that kind of auto-generating though, so we should probably also consider whether that's something we want to do, or if we should be doing something different like exposing Jinja2 macros or filter functions instead.

Finally, another issue is that we had already used the version name in templates. This referred to the version of Nengo bones, so I changed that to bones_version, which I think we should do regardless. However, we could also consider changing this key from version to project_version or something like that to make it explicit both ways.

Interactions with other PRs:
Based on #38, could be made independent if people prefer.

How has this been tested?
Modified tests and used it to generate files in Nengo Bones.

How long should this take to review?

  • Average (neither quick nor lengthy)

Where should a reviewer start?
Probably the new .nengobones.yml, and version.py/version.py.template.

Types of changes:

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Still to do:

  • Decide on discussion points above
  • Implement decision
  • Update documentation
  • Add some helper functions to tests to make generating test .nengobones.yml files easier
  • Add changelog entry
  • Change base of this PR back to master once Add gitignore template #38 is merged

@drasmuss
Copy link
Member

We should add something to this to include the copyright dates in version.py (to address #19).

@drasmuss
Copy link
Member

Done in #151

@drasmuss drasmuss closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants