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

Wrong type hint for createEnumType #1378

Closed
headtr1ck opened this issue Oct 27, 2024 · 3 comments
Closed

Wrong type hint for createEnumType #1378

headtr1ck opened this issue Oct 27, 2024 · 3 comments

Comments

@headtr1ck
Copy link
Contributor

It seems that createEnumType is wrongly typed as:

def createEnumType(
    self,
    datatype: np.dtype[np.integer] | type[np.integer] | type[int],
    datatype_name: str,
    enum_dict: Mapping[str, int | np.integer],
) -> EnumType: ...

But e.g. datatype="u1" is also supported.

  • the version of the software with which you are encountering an issue: netCDF4==1.7.2
  • environmental information (i.e. Operating System, compiler info, java version, python version, etc.): Win10 / python 3.12
  • a description of the issue with the steps needed to reproduce it:
    downstream issue:
    Mypy 1.12 issues pydata/xarray#9624

problematic lines (roughly):

with nc4.Dataset(tmp_file, mode="w") as nc:
    nc.createEnumType("u1", "cloud_type", {"clear": 0, "cloudy": 1})
@jswhit
Copy link
Collaborator

jswhit commented Oct 29, 2024

so what's the suggested fix?

@jswhit
Copy link
Collaborator

jswhit commented Oct 29, 2024

@RandallPittmanOrSt

@RandallPittmanOrSt
Copy link
Contributor

RandallPittmanOrSt commented Oct 29, 2024

Good catch. I think the most expedient correct fix is to just add str to the union for datatype and rely on the runtime (_def_enum() ) to ensure that it's an integer type. So the signature is just

def createEnumType(
    self,
    datatype: np.dtype[np.integer] | type[np.integer] | type[int] | str,
    datatype_name: str,
    enum_dict: Mapping[str, int | np.integer],
) -> EnumType: ...

Looks like we got this right on EnumType.__init__() so it was an oversight here.

I don't think adding a Literal would be helpful here (especially based on our discussion here)

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

No branches or pull requests

3 participants