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

Remove compat.py and version.py #623

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Conversation

zariiii9003
Copy link
Collaborator

  • remove compat.py
  • remove version.py
  • update test dependencies
  • test on CPython 3.12
  • fix FutureWarnings and DeprecationWarnings

@zariiii9003 zariiii9003 changed the title Remove compat,py and version.py Remove compat.py and version.py Nov 15, 2023
@coveralls
Copy link

coveralls commented Nov 15, 2023

Pull Request Test Coverage Report for Build 6894506838

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on remove_compat at 93.56%

Totals Coverage Status
Change from base Build 6702898615: 93.6%
Covered Lines: 21677
Relevant Lines: 23169

💛 - Coveralls

Copy link
Member

@andlaus andlaus left a comment

Choose a reason for hiding this comment

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

looks good. I'd really like to do a release by just creating a tag on github, though...


# Remove once less users are using the old package structure.
from . import database as db # isort: skip

__author__ = 'Erik Moqvist'
__version__ = '39.3.0'
Copy link
Member

@andlaus andlaus Nov 15, 2023

Choose a reason for hiding this comment

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

putting this here will not come with a smaller a maintainance burden than having version.py. maybe we can use something like setuptools_scm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The maintenance is the same as before. But the version.py file is just a leftover from the time, when __version__ was parsed inside setup.py. With pyproject.toml it serves no purpose.

Regarding setuptools_scm, i have no clue what that does 😄

Copy link
Member

Choose a reason for hiding this comment

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

it basically determines at the latest tagged version in the git history and based on that creates a version.py file when making a pypi package or running pip install -e .. the only drawback is that it requires the build system to be based on setuptools (I think), but as far as I know, this is the case for cantools. (right?)

Copy link
Member

Choose a reason for hiding this comment

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

(I can make a patch if this sounds interesting to you...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I can make a patch if this sounds interesting to you...)

Sure, i don't mind.

it basically determines at the latest tagged version in the git history and based on that creates a version.py file when making a pypi package or running pip install -e ..

The file is optional though, right? I think the __version__ value can be set dynamically with importlib.metadata

the only drawback is that it requires the build system to be based on setuptools (I think), but as far as I know, this is the case for cantools. (right?)

Yes, it's setuptools.

Copy link
Member

Choose a reason for hiding this comment

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

okay, I made a PR for your PR: zariiii9003#1

it's probably not optimal, but it seems to work ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then i'd prefer the no file implementation with importlib 😄

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but since this is quite a bit away from my home turf, I'm not sure how to do this. (any hints?)

Copy link
Member

Choose a reason for hiding this comment

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

after a bit of digging. I found out that the importlib.metadata path already works with the patch I proposed:

>>> from importlib.metadata import version
>>> version('cantools')
'39.3.1.dev6+g20191b97'

that said, I'm a bit ambivalent of whether it is a good idea to set the __version__ variable in __init__py this way because it makes things quite twisted...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I merged your PR and updated it to use importlib.metadata. It works fine.

andlaus and others added 3 commits November 16, 2023 11:27
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Florian Jost <florian.jost@mbition.io>
use setuptools_scm to determine the version based on the latest git tag
@andlaus
Copy link
Member

andlaus commented Nov 16, 2023

cool, merging.

@andlaus andlaus merged commit 99eac2f into cantools:master Nov 16, 2023
16 checks passed
@zariiii9003 zariiii9003 deleted the remove_compat branch November 16, 2023 18:01
@andlaus
Copy link
Member

andlaus commented Nov 16, 2023

I just tagged the 39.4.0 release, mainly to see if everything works as expected. it does :)

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.

None yet

3 participants