Skip to content

Conversation

@pfbuxton
Copy link

@pfbuxton pfbuxton commented Mar 4, 2025

I have removed all internal wildcard imports.

While doing this I noticed what looks to be a bug. DTYPE_RANGE is defined twice: once as a variable, and once as a function:

DTYPE_RANGE = 201

def DTYPE_RANGE(*args):

My implementation of mdsthin/__init__.py is absolutely horrible! Nearly every variable, function, and class is exposed to the public. I have not made any change in what is exposed to the public, so the behaviour is identical. In my opinion nearly all of this should be removed. However, I didn't want to decide what to expose and what not to. Perhaps it would be best to leave mdsthin/__init__.py unaltered with the wildcard imports left, until a decision is made on what should be exposed to the public?

The imports are all formatted using ruff check --select I --fix <file_name> this alphabetically sorts and groups the imports by: standard library, blank line, third party, blank line, internal imports.

I also added a few extra tests. All tests passed on Python 3.7 - 3.13. However, I didn't run any of the ssh tests as we don't use ssh, and test_root_whoami never worked for me due to custom server settings.

Hope some of this is of help.

@WhoBrokeTheBuild
Copy link
Member

Thank you again for all of the hard work!

First of all, I would like to break this up into a series of PRs so we can work on the individual modifications, this is more than just the wildcards at the moment, it also includes typing and modifications to the configuration and much more.

So, just focusing on the wildcards for now, I personally believe that the public API should be almost all of the package, with the exception of what's in internals/ probably. We can probably avoid exporting the internal lookup *_MAP variables as well. However, maybe just going back to the wildcards for __init__.py is more reasonable.

I would expect a regular user to import mdsthin or from mdsthin import Connection and a more advanced user could make use of the more granular types.

Oh and DTYPE_RANGE is less of a bug and more of an.. unfortunate coincidence. There is, in fact, both a DTYPE_RANGE in the C code that defines the DType ID for Range, and a TDI builtin of the same name. There will need to be some way to differentiate them I suppose, maybe just a trailing _ in the TDI function version.. I'll have to think about it.

@WhoBrokeTheBuild WhoBrokeTheBuild self-assigned this Mar 4, 2025
@pfbuxton
Copy link
Author

I completely agree. I will split this up into a PR with only the internal imports changed.

My implementation of __init__.py was absolutely horrible. So I agree the wildcard imports within __init__.py is the best solution. But it does nicely show that hundreds of variables are being exposed. The only downside of reverting __init__.py is that standard library and third party packages would then also be exposed, e.g. ctypes and numpy. This is why code like this works without errors:

from MDSplus import *  # this will import `ctypes` and `numpy`

x = numpy.linspace(0.0, 1.0, 100)
conn = Connection("<your server>")

but as you say if most people are using import mdsthin or from mdsthin import Connection then there is no problem.

I'll get the updated PR mid week (probably).

@WhoBrokeTheBuild
Copy link
Member

Thanks again!

And aren't there rules about underscores and wildcards? What if we alias the imports of numpy and ctypes to _numpy and _ctypes, and change any of the internal things to do the same, I think that would allow the wildcard imports to work normally? I'll have to check.

@pfbuxton
Copy link
Author

pfbuxton commented Mar 10, 2025

Would be easier to delete the objects which you don't want exposed.

__init__.py would then be:

from .connection import *
from .descriptors import *
from .exceptions import *
from .functions import *

# Delete system standard library and third party packages, which should not be exposed to users
del ctypes
del getpass
del logging
del numpy
del os
del socket
del sys
del time

@WhoBrokeTheBuild
Copy link
Member

Oh that's an interesting idea, I think I could live with that. It mirrors what I do for the fake MDSplus import as well:
https://github.com/MDSplus/mdsthin/blob/main/mdsthin/MDSplus/__init__.py#L43

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.

2 participants