Add version info to each generated file #144
Conversation
There was a problem hiding this comment.
Could we call this file something which better describes it's reason for being?
Maybe generated_by_tags.py?
There was a problem hiding this comment.
It's fine as it is. If it grows and requires refactoring, then so be it. It's similar to the Django convention of putting views in a views.py file, models in a models.py file etc.
There was a problem hiding this comment.
Actually, version_tag.py, to match the class name, would be best.
There was a problem hiding this comment.
jinja2 tags are a standard. It's self explanatory and follows conventions.
There was a problem hiding this comment.
Everywhere I have worked for over a decade it has been the convention to name a file containing a class to match the class name.
Are you able to link to the standard you mention?
There was a problem hiding this comment.
It sounds like you've spent too much time working with Java; my experience has been the opposite for the past 10+ years working mainly in python land.
It's worth reading the modules section of this python structure guide: https://docs.python-guide.org/writing/structure/#modules
Maybe we should start a team style guide and come to a consensus on these smaller issues, so that we can increase the objectivity of code reviews?
There was a problem hiding this comment.
Ignoring the side languages, the last ten years went PHP > TypeScript > Java & Ruby.
Now Python.
I am 100% up for a team style guide. I'll start a thread in Slack to get the ball rolling.
| def _get_version(self): | ||
| try: | ||
| return version("dbt-copilot-tools") | ||
| except PackageNotFoundError: |
There was a problem hiding this comment.
Is it possible to test this? Looks like we are testing the happy path via fixtures already, but we also handle this exception and return "UNKNOWN".
There was a problem hiding this comment.
It should be an edge case - it only happens if the code is run directly, instead of after installing the package. I did consider manually parsing pyproject.toml to grab the version, but I don't see much value in it. Your --version command also has the same issue.
There was a problem hiding this comment.
The reason why I mention it is because we have a similar method here that does parse pyproject.toml: https://github.com/uktrade/copilot-tools/blob/main/utils/check_pypi.py#L43
There was a problem hiding this comment.
Oh cool, I'll raise another PR and fix it in both places.
There was a problem hiding this comment.
OK, I went down a rabbit hole here. I wasn't liking the way Poetry installs the package as a dependency so at one point I manually removed it, which then caused the PackageNotFound exception.
I did some digging yesterday and found this: python-poetry/poetry#800, which explains why poetry works this way.
But the upshot of it is, there's no scenario that version() should fail and require a fall back to parsing the toml file. So I'm just going to remove the try/except block.
There was a problem hiding this comment.
Yeahhh, I was thinking we would ideally never run into that exception anyway. It might be worth bumping up the version as well, so this change gets published on to PyPI as well.
This PR adds logic that drops a comment at the top of every generated file including the copilot-helper version and the date/time that the file was created.
The
copilot/.workspacefile has been omitted because its contents does not vary.Example comment: