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

Fix type errors #105

Merged
merged 14 commits into from
Nov 25, 2022
Merged

Fix type errors #105

merged 14 commits into from
Nov 25, 2022

Conversation

cbrnr
Copy link
Owner

@cbrnr cbrnr commented Aug 1, 2022

Fixes #93.

@cbrnr
Copy link
Owner Author

cbrnr commented Aug 1, 2022

In my opinion, the remaining errors/warnings (when setting "python.analysis.typeCheckingMode": "basic") are useless. These include:

  • values that can be None, even if we explicitly take care of that literally in the previous line
      if classifiers_dir is None:
          classifiers_dir = get_config("classifiers_dir")
    
      target_file = Path(classifiers_dir).expanduser() / name
  • things like ax[row], where Pylance says that we cannot index a Matplotlib Axes,
  • and NumPy-related stuff where I cannot even read the error message because it is basically gibberish.

@hofaflo let me know if you think we should address any of the remaining errors (and if so, please show me how).

@cbrnr
Copy link
Owner Author

cbrnr commented Aug 1, 2022

Ima turn off type checking now to stay sane.

@hofaflo
Copy link
Collaborator

hofaflo commented Nov 19, 2022

Pyright will still complain, but I think making mypy happy is enough :)

The mkdocstrings issue is already fixed upstream (mkdocstrings/python#45), so I bumped the dependency. 5e81986 should have worked around this, but for some reason readthedocs ignored the changed path to the configuration file and kept calling mkdocs with --config-file mkdocs.yml instead of --config-file ./mkdocs.yml (see the build log) 🤷

@hofaflo
Copy link
Collaborator

hofaflo commented Nov 20, 2022

This is ready to merge from my point of view @cbrnr :)

Copy link
Owner Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Looks better than expected 😄! I have a couple of questions (inline) plus the following two points:

  1. Regarding the py.typed file – why is this empty file necessary (what does it do)? And if it is necessary, should it not be included as package_data?
  2. I think we should add .mypy_cache to .gitignore.

sleepecg/io/physionet.py Show resolved Hide resolved
sleepecg/io/gudb.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@cbrnr
Copy link
Owner Author

cbrnr commented Nov 21, 2022

@hofaflo, besides my minor comments this looks really good (I've edited my comments so that all questions are in the first comment).

@hofaflo
Copy link
Collaborator

hofaflo commented Nov 24, 2022

Regarding the py.typed file – why is this empty file necessary (what does it do)?

PEP561 says so 🤷. I'm not totally sure but I assume it tells type checkers (such as mypy) that the package is typed.

And if it is necessary, should it not be included as package_data?

Totally, it is included in the wheels and in the source archive (I guess because its parent directory is listed in MANIFEST.in?).

I think we should add .mypy_cache to .gitignore.

For some reason my VS Code already ignored it so I didn't notice, but sure, I'll add it :D

@cbrnr
Copy link
Owner Author

cbrnr commented Nov 25, 2022

Looks great! Ready to merge?

@hofaflo
Copy link
Collaborator

hofaflo commented Nov 25, 2022

Yes!

@cbrnr cbrnr merged commit 42291f8 into main Nov 25, 2022
@cbrnr cbrnr deleted the fix-type-errors branch November 25, 2022 08:05
@cbrnr
Copy link
Owner Author

cbrnr commented Nov 25, 2022

Thanks @hofaflo!

@hofaflo hofaflo mentioned this pull request Nov 25, 2022
1 task
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.

Fix type annotation issues
2 participants