-
Notifications
You must be signed in to change notification settings - Fork 36
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: Add version number and checks #328
base: FABulous2.0-development
Are you sure you want to change the base?
feat: Add version number and checks #328
Conversation
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.
Cool, thanks for this! :) The fabulous version that is written to the file is a bit weird (it shows 0.0.post754
for me), but I guess this due to the current python setup?
I am showing Let's delay the merge until we do the release. I don't want to rebase again. |
Okay then probably I did something wrong although I just cloned your fork and checked out your Delaying the merge might make sense as I think you put a lot of effort into the rebase and this should indeed not be done too often :) |
This could be due to the branch checkout. But ultimately, this should not matter. |
Okay so you also implemented the change in the master? Or what do you mean? As you said it does not matter but I personally would like to understand what happens and if I probably did something wrong. |
I implemented the change in the development. The installation will depend on the |
You did nothing wrong, for a clean venv with Kelvins branch, it should be The version Kelvin showed, seems to have some artifacts from whenever he installed FABulous in his venv. It's all based on the git history. The first numbers Here can you find more information about versioning: https://setuptools-git-versioning.readthedocs.io/en/stable/schemas/tag/dirty_version.html |
@@ -131,6 +132,12 @@ def setup_project_env_vars(args: argparse.Namespace) -> None: | |||
) | |||
os.environ["FAB_PROJ_LANG"] = args.writer | |||
|
|||
# string comparison | |||
if os.environ["VERSION"] < version("FABulous-FPGA"): |
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 think it's a bit too much simplified, if we just check if project version is not newer than your current fabulous version. Older fabulous versions can't handle newer projects as well.
I'm also not sure, if we should check for the whole version string, technically breaking changes should only happen in major releases.
I think we should at least not throw warnings for patch, and dirty versions.
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 will make the changes once we decide what we want for versioning. Probably using something like packaging should allow us to do more fine control.
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.
This is a really nice improvement!
We should discuss, how we treat the versioning for the checks.
Add the package version when creating a project. If the project version is less than the starting version, add a warning during package setup.
It can further improve to raise errors when the project version and the package version have major version differences. But this can be done later.