Skip to content

Conversation

@banesullivan
Copy link
Owner

Follow up to #101 to use importlib.metadata as it is preferred. This refactors get_version() and the knowledge module quite a bit to remove unneeded logic as importlib.metadata.version can do much of (all?) what we need!

Further this, necessarily, drops support for Python 3.7.

Do not merge until we are ready to drop Python 3.7 support, which, IMO should match when PyVista drops 3.7 as that's the primary downstream dependency from my perspective

@prisae
Copy link
Collaborator

prisae commented Jan 25, 2023

Earliest in July 2023: https://devguide.python.org/versions/#supported-versions (not that far away actually)

@banesullivan
Copy link
Owner Author

@prisae, so now importlib.metadata cannot find the dummy_module which makes sense, because it is not installed in the environment.... I'm tempted to remove that and say it's an invalid scenario. But first, I'd like to do some testing to make sure all of this works with conda as well

@prisae
Copy link
Collaborator

prisae commented Jan 25, 2023

It should work. And I don't think it is that an uncommon scenario, in my experience working with people...

@banesullivan
Copy link
Owner Author

But first, I'd like to do some testing to make sure all of this works with conda as well

All works with conda-installed packages

Copy link
Collaborator

@prisae prisae left a comment

Choose a reason for hiding this comment

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

A few comments, take what you like and leave the rest.

Comment on lines 278 to 279
A couple of locations are checked, and we are happy to implement more if
needed, just open an issue!
Copy link
Collaborator

Choose a reason for hiding this comment

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

A couple of locations are checked, and we are happy to implement more if
needed, just open an issue!

This could potentially also be removed.

Similarly, `VERSION_METHODS` is a dictionary for methods to retrieve the
version, and you can similarly add your methods which will get the version
of a package.
Currently, it uses `importlib.metadata.version` to get the distribution version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the entire section could be removed. Anyway, it is funny, as it will shift the focus of scooby. A big part of scooby was to find the version number in all potential odd situation. Which was sort of the detective part. Now, using importlib, scooby sort of (at least for the versions) is merely a display tool. Which is not a bad thing at all, but it is interesting to see where the whole thing came from and where it goes.

uses: actions/setup-python@v2
with:
python-version: "3.9"
python-version: "3.11"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there would be a "latest" or "latest-1" flag or so, such that this has not to be manually adjusted every now and then. (Nothing to do with this PR though.)

fail-fast: false
matrix:
python-version: [3.7, 3.8, 3.9, "3.10", "3.11"]
python-version: [3.8, 3.9, "3.10", "3.11"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there would be a "supported" or "official" flag or so, such that this has not to be manually adjusted every now and then. (Nothing to do with this PR though.)

Currently, it uses `importlib.metadata.version` to get the distribution version.

### Using scooby to get version information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The following lines will potentially have to be adjusted:

>>> scooby.get_version('no_version')
('no_version', 'Version unknown')
>>> scooby.get_version('does_not_exist')
('does_not_exist', 'Could not import')

MODULE_NOT_FOUND = 'Module not found'
MODULE_TROUBLE = 'Trouble importing'
VERSION_NOT_FOUND = 'Version unknown'
NOT_PROPERLY_INSTALLED = '(not properly installed)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This statement has a tone of judgement, and I would prefer scooby to not be opinionated about how users do things, I do not think it is the scope of scooby.

Having a local module is a valid scenario, and if a users decides to do so it is not helpful by always slapping in its face "not properly installed". E.g., a folder full of notebooks, with a small local "module"-folder in it that is accessed by all notebooks. This is only meant to be local, never used anywhere else. This might even be versioned. I think that happens more than you might think.

As such, I would prefer a short string, free of judgement. E.g., (local), (sys.path), or similar.

if ver is not None:
return name, ver
return name, f'{ver} {NOT_PROPERLY_INSTALLED}'
return name, f'{VERSION_NOT_FOUND} {NOT_PROPERLY_INSTALLED}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am always reluctant too have to many returns, but that is personal preference. What about

        if ver is None:
            ver = VERSION_NOT_FOUND
        ver += f' {NOT_PROPERLY_INSTALLED}'

and then it is returned at the end?

@prisae
Copy link
Collaborator

prisae commented Jan 26, 2023

It is an interesting PR. So ideally every "detective" work within scooby should be replaced by things from the standard library, if they are implemented. So scooby will become, over time, less and less "detective" and reduces to "report". It is a good long-term goal! ;-)

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