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

Add py.typed Marker #2904

Merged
merged 5 commits into from Jul 2, 2022
Merged

Conversation

adam-grant-hendry
Copy link
Contributor

@adam-grant-hendry adam-grant-hendry commented Jun 29, 2022

This will allow static type checkers to recognized the package's type hints per PEP-561

Fixes issue #2903

This will allow static type checkers to recognized the package's type hints per PEP-561

Fixes issue pyvista#2903
@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jun 29, 2022
@adam-grant-hendry
Copy link
Contributor Author

@pyvista/developers Would you please review and approve this PR?

@adam-grant-hendry
Copy link
Contributor Author

@akaszynski Can you confirm you get pinged when I enter @pyvista/developers ? I'm not sure it's working for me.

@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #2904 (f62328c) into main (dcf18ff) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2904      +/-   ##
==========================================
- Coverage   94.11%   94.06%   -0.05%     
==========================================
  Files          76       76              
  Lines       16521    16525       +4     
==========================================
- Hits        15549    15545       -4     
- Misses        972      980       +8     

@adam-grant-hendry
Copy link
Contributor Author

@tkoyama010 Just confirming, did you receive a ping about this PR? I'm not sure if the pyvista/devlopers option is working for me.

@tkoyama010
Copy link
Member

@adam-grant-hendry I think it doesn't work with your permission. But if you post the Pull Request, most of them will review it.

@adam-grant-hendry
Copy link
Contributor Author

@adam-grant-hendry I think it doesn't work with your permission. But if you post the Pull Request, most of them will review it.

Thanks for confirming. Would I be able to get permission somehow?

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

I am not very familiar with this. But do we need to include this in the package_data in setup.py as well?

@tkoyama010
Copy link
Member

Thanks for confirming. Would I be able to get permission somehow?

It belongs to the @akaszynski .

@adam-grant-hendry
Copy link
Contributor Author

Thanks for confirming. Would I be able to get permission somehow?

It belongs to the @akaszynski .

Ah no problem then. 👍

@adam-grant-hendry
Copy link
Contributor Author

I am not very familiar with this. But do we need to include this in the package_data in setup.py as well?

Oh yes, you are right. Good catch! Per PEP 561:

To have this file installed with the package, maintainers can use existing packaging options such as package_data in distutils, shown below.

I'll update the setup.py.

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

Do we use annotations from PyVista itself, or do we add stub files?

@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Jun 29, 2022

Do we use annotations from PyVista itself, or do we add stub files?

We place them in the pyvista source itself. These are the typical function and variable annotations you see, e.g.

x: int = 1

If we used stubs, they would be files with the *.pyi extension and have to be placed next to the *.py files in the package.

pyvista/py.typed Outdated Show resolved Hide resolved
@adam-grant-hendry
Copy link
Contributor Author

As a follow-up, stubs are typically used instead of annotations directly in the source code for two reasons:

  1. To create a "stub-only" package. The stubs have nothing but annotations and no implementation details (similar to a C-header). This separates the annotations from the source, which some 3rd-party packages prefer. It allows the two to be developed separately, but it can make aligning signatures across the packages more difficult. Types are also already placed into docstring documentation in well-maintained packages, so it makes sense to add the annotations to the source anyway (tools like vscode's extension autoDocstring can pick these up automatically and put them in for you, which is nice).

(I believe I asked, or was going to ask, @akaszynski which method he would prefer, but I saw the annotations were already in the source and we already type check with mypy in our pre-commit, so I figured out this was what we want).

  1. To override annotations in the source code. Per PEP 561, the module resolution order for static type checkers is:
$PATH/$MYPYPATH -> User code -> Stub packages -> Inline packages -> Typeshed

so stubs can be used to patch broken stubs from packages:

Type checkers SHOULD provide this to allow the user complete control of which stubs to use, and to patch broken stubs/inline types from packages. In mypy the $MYPYPATH environment variable can be used for this.

tkoyama010
tkoyama010 previously approved these changes Jun 29, 2022
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the explanation!

@adam-grant-hendry
Copy link
Contributor Author

LGTM. Thanks for the explanation!

You bet! Thanks for asking and thanks for the approval!

@akaszynski
Copy link
Member

Let it be known that type annotations is still a work in progress and we don't have it in most places. @adam-grant-hendry, if you feel up to it, you can feel free to add them. I'd do it slowly across several PRs. Tracking down mypy errors usually drives me up a wall.

@adam-grant-hendry
Copy link
Contributor Author

Let it be known that type annotations is still a work in progress and we don't have it in most places. @adam-grant-hendry, if you feel up to it, you can feel free to add them. I'd do it slowly across several PRs. Tracking down mypy errors usually drives me up a wall.

😄 It does for me too! Yes, I am happy to track them down. For better or worse, type hints appear here to stay in Python, and I think it will help us in the long run as the package grows.

Can we approve this PR though?

  1. Without a py.typed marker, mypy won't see the annotations from pyvista, so I won't be able to track down problems in pyvistaqt (I'll get the infamous Stub file not found for "package" error).

  2. I added the partial\n indicator to the package to indicate that it is partially typed, which will pull information from inline packages and typeshed where possible.

Completely fine that is WIP and happy to implement slowly over multiple PRs. Shouldn't break any runtime functionality of pyvista. This marker will mostly help type hinting development of pyvista subpackages that import it (like the docs and pyvistaqt).

@adam-grant-hendry
Copy link
Contributor Author

@akaszynski @adeak @tkoyama010 Have we achieved consensus yet on adding the py.typed marker? This shouldn't affect anything, it will only help us.

Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

I've never heard of py.typed, but I don't really know typing, and the primer from mypy you linked in #2903 seems clear as day: mypy needs this, and it can't cause any harm.

One question though: the same page also has a note:

If you use setuptools, you must pass the option zip_safe=False to setup(), or mypy will not be able to find the installed package.

We use setuptools and we don't have zip_safe=False. Do we need this too, and what else would passing this kwarg change?

@adeak
Copy link
Member

adeak commented Jun 30, 2022

It also says

The instructions above are enough to ensure that the built wheels contain the appropriate files. However, to ensure inclusion inside the sdist (.tar.gz archive), you may also need to modify the inclusion rules in your MANIFEST.in:

global-include *.pyi
global-include *.typed

Since we have an sdist on PyPI we probably have to do this too.

@akaszynski
Copy link
Member

It also says

The instructions above are enough to ensure that the built wheels contain the appropriate files. However, to ensure inclusion inside the sdist (.tar.gz archive), you may also need to modify the inclusion rules in your MANIFEST.in:

global-include *.pyi
global-include *.typed

Since we have an sdist on PyPI we probably have to do this too.

Let's add this.

@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Jul 1, 2022

We use setuptools and we don't have zip_safe=False. Do we need this too, and what else would passing this kwarg change?

Great question! First, we really should start following PEP 518 and specify that our build backend is setuptools in our pyproject.toml, like this:

[build-system]
requires = [
    "setuptools",
    "wheel",
    "toml",
]
build-backend = "setuptools.build_meta"

The original purpose of the pyproject.toml file was to specify what the project build backend is. It has a great rationale as to why it's required now.

In addition, thanks to tool tables from the PEP, projects have now begun using the pyproject.toml to specify configuration options for different packages. This is great because now we can have a single source of truth, much like the setup.cfg file provides, for those projects that support adding configuration details to the pyproject.toml. In our case, I can eliminate all but the .flake8 and .codespellrc configuration files:

  • .coverage.rc
  • .isort.cfg
  • .pylintrc
  • mypy.ini
  • pytest.ini

and put them in the pyproject.toml.

It seems we have (rather innocently) used the pyproject.toml because black requires its use for configuring black, but haven't specified what the build backend is. Brett Cannon wrote a great blog post about this, namely:

Recently on Twitter there was a maintainer of a Python project who had a couple of bugs filed against their project due to builds failing (this particular project doesn't provide wheels, only sdists). Eventually it came out that the project was using a pyproject.toml file because that's how you configure Black and not for any other purpose. This isn't the first time I have seen setuptools users use pyproject.toml because they were "told to by " without knowing the entire point behind the file. And so I decided to write this blog post to try and explain to setuptools users why pyproject.toml exists and what it does as it's the future of packaging in the Python ecosystem (if you are not a conda user 😉).
...
I've blogged about this before, but the purpose of PEP 518 was to come up with a way for projects to specify what build tools they required. That's it, real simple and straightforward.

Here's how we could put all the configurations I mentioned into our pyproject.toml (also using PEP 621 for our project metadata):

pyproject.toml
[project]
# PEP 621 project metadata
# See https://www.python.org/dev/peps/pep-0621/
authors = [
    { name = "PyVista Developers", email: "info@pyvista.org" },
]
# Alternative to below, if we don't want the `pyproject.toml` to keep the
# version single source of truth and instead keep it in the `_version.py`
# file, we can remove the bottom two entries and use
#
# [tool.setuptools.dynamic]
# version = { attr = "pyvista._version.__version___" }
# readme = { file = ["README.rst"] }
version = "v0.34.2"
readme = "README.rst"
requires-python = ">=3.7"
license = { text = "MIT" }
name = "pyvista"
description = "Easier Pythonic interface to VTK"
classifiers = [
    'Development Status :: 4 - Beta',
    'Intended Audience :: Science/Research',
    'Topic :: Scientific/Engineering :: Information Analysis',
    'License :: OSI Approved :: MIT License',
    'Operating System :: Microsoft :: Windows',
    'Operating System :: POSIX',
    'Operating System :: MacOS',
    'Programming Language :: Python :: 3.7',
    'Programming Language :: Python :: 3.8',
    'Programming Language :: Python :: 3.9',
]
keywords = "vtk numpy plotting mesh"
dependencies = [
    "numpy",
    "imageio",
    "pillow",
    "appdirs",
    "scooby>=0.5.1",
    "vtk"
]

[project.optional-dependencies]
all = [
    "matplotlib",
    "colorcet",
    "cmocean",
    "meshio>=5.2",
    "ipyvtklink",
    "pythreejs"
]
colormaps = [
    "matplotlib",
    "colorcet",
    "cmocean"
]
io = [
    "meshio>=5.2"
]
jupyter = [
    "ipyvtklink",
    "pythreejs"
]
docs = [
    "Sphinx==4.5.0",
    "cmocean==2.0",
    "colorcet==3.0.0",
    "imageio-ffmpeg==0.4.7",
    "imageio>=2.5.0",
    "ipygany==0.5.0",
    "ipyvtklink==0.2.2",
    "jupyter_sphinx==0.4.0",
    "jupyterlab==3.4.3",
    "lxml==4.9.0",
    "matplotlib==3.5.2",
    "meshio==5.3.4",
    "mypy-extensions==0.4.3",
    "mypy==0.961",
    "numpydoc==1.4.0",
    "osmnx==1.2.1",
    "panel==0.13.1",
    "param==1.12.2  # due to panel bug",
    "pydata-sphinx-theme==0.9.0",
    "pypandoc==1.8.1",
    "pytest-sphinx==0.4.0",
    "pythreejs==2.3.0",
    "scipy==1.8.1",
    "sphinx-autobuild==2021.3.14",
    "sphinx-copybutton==0.5.0",
    "sphinx-gallery==0.10.1",
    "sphinx-notfound-page==0.8",
    "sphinx-panels==0.6.0",
    "sphinxcontrib-websupport==1.2.4",
    "trimesh==3.12.6",
    "typed-ast==1.5.4",
    "typing_extensions==4.2.0"
]
# Note: using < here to reduce breakages
# These should be automatically kept up to date by dependabot
test = [
    "Sphinx<4.6.0",
    "cmocean<2.1",
    "codecov<2.2.0",
    "colorcet<3.1.0",
    "hypothesis<6.48.2",
    "imageio-ffmpeg<0.5.0",
    "imageio<2.20.0",
    "ipygany<0.6.0",
    "ipython<9.0.0",
    "ipyvtklink<0.4.0",
    "itkwidgets<0.33.0; python_version < '3.10'",
    "matplotlib<3.6.0",
    "meshio<5.4.0",
    "panel<0.14.0",
    "param<1.13.0",
    "pytest-cov<3.1.0",
    "pytest-memprof<0.3.0",
    "pytest-xdist<2.6.0",
    "pytest<7.2.0",
    "pythreejs<2.4.0",
    "sphinx-gallery<0.11.0",
    "tqdm<4.65.0",
    "trimesh<3.13.0"
]

[project.urls]
homepage = "https://www.pyvista.org/"
repository = "https://github.com/pyvista/pyvista"
documentation = "https://docs.pyvista.org/"
bug_tracker = "https://github.com/pyvista/pyvista/issues"

[tools.setuptools]
packages = [
    "pyvista",
    "pyvista.examples",
    "pyvista.core",
    "pyvista.core.filters",
    "pyvista.demso",
    "pyvista.jupyter",
    "pyvista.plotting",
    "pyvista.utilities",
    "pyvista.ext"
]

[tools.setuptools.package_data]
pyvista = [
    "py.typed"
]
pyvista.examples = [
    "airplane.ply",
    "ant.ply",
    "channels.vti",
    "hexbeam.vtk",
    "sphere.ply",
    "nut.ply",
    "uniform.vtk",
    "rectilinear.vtk",
    "globe.vtk",
    "2k_earth_daymap.jpg"
]

# Package Configurations
# ======================
# For ease of lookup, I try to keep these in alphabetical order

[tool.black]
line-length = 100
skip-string-normalization = true
target-version = ["py39"]
# `pre-commit` doesn't see skips; `force-exclude` forces it
# to see these files
#
# See:
#   - https://github.com/psf/black/issues/1584
force-exclude = '''
(
    pyvista/.*/__init__.py
  | pyvista/__init__.py
)
'''

[tool.pydocstyle]
match = '''
/(
  (?!
      (
        ^pyvista/ext/
      )
  )
  |  pyvista/
  | other/
/)
'''

[tool.coverage.run]
branch = true
# `shiboken6`, which creates the python bindings for `Qt` C++ source, imports from a
# `zip` file into the top-level directory at runtime. These files are deleted after
# running, but `coverage` attempts to look at their source after they're gone, causing
# warnings to appear. Namely, it looks for these modules/files:
#
#    project_dir/pysrcript
#    project_dir/shibokensupport
#    project_dir/signature_bootstrap.py
#
# To avoid this error, `source` is specified to the package subdirectory. However, this
# can also be avoided by explicitly omitting these folders in the `omit` section.
#
# See https://github.com/nedbat/coveragepy/issues/1392
source = [
    'pyvista/'
]
omit = [
    'pyvista/ext/coverage.py',
    'pyvista/conftest.py',
    # kept for backwards compatibility:
    'pyvista/plotting/theme.py'
]
disable_warnings = ['no-data-collected']

[tool.coverage.report]
exclude_lines = [
    'pragma: no cover',
    'def __repr__',
    'raise AssertionError',
    'raise NotImplementedError',
    'if __name__ == .__main__.:',
    '@(abc\.)?abstractmethod'
]

[tool.isort]
profile = "black"
line_length = 100
add_imports = [
    # "from __future__ import annotations"  # Automatically add to module on save, if not there
]
# Sort by name, don't cluster "from" vs "import"
force_sort_within_sections = true
# Combines "as" imports on the same line
combine_as_imports = true
# Prevent linter ignore comments from being moved off the offending line
ensure_newline_before_comments = true
# Some of Adam's favorites, for consideration
# multi_line_output = 3
# include_trailing_comma = true
# force_grid_wrap = 0
# use_parentheses = true

[tool.mypy]
ignore_missing_impoorts = true
# More of Adam's favorites...
# python_version = "3.9"
# disallow_untyped_defs = true
# warn_return_any = true
# warn_unused_configs = true
# warn_unused_ignores = true
# warn_redundant_casts = true
# show_error_codes = true
# no_pretty = true
# show_column_numbers = true
# plugins = [
#    "pydantic.mypy"
# ]
# exclude = [
#     'venv',
#     '\.venv/',
#     'build/',
#     '_build/',
#     'dist/',
#     'ci/'
# ]
# fast_module_lookup = true

[[tool.mypy.overrides]]
module = "vtk.*"
ignore_missing_imports = true

[[tool.mypy.overrides]]
module = "matplotlib.*"
ignore_missing_imports = true

[[tool.mypy.overrides]]
module = "numpy.*"
ignore_missing_imports = true

[[tool.mypy.overrides]]
module = "scooby.*"
ignore_missing_imports = true

[[tool.mypy.overrides]]
module = "appdirs.*"
ignore_missing_imports = true

[[tool.mypy.overrides]]
module = "itkwidgets.*"
ignore_missing_imports = true

[tool.pydocstyle]
convention = "numpy"

[tool.pylint.master]
disable = """
    raw-checker-failed,
    bad-inline-option,
    locally-disabled,
    file-ignored,
    suppressed-message,
    useless-suppression,
    deprecated-pragma,
    use-symbolic-message-instead,
    arguments-differ,
    no-name-in-module,
    no-member,
    # Redundant alias imports required for type hinting by PEP 484
    useless-import-alias,
    # Qt uses PascalCase
    invalid-name,
"""
extension-pkg-allow-list = """
    vtk,
    PyQt5,
    PyQt6,
    PySide2,
    PySide6
"""
ignore_patterns = '''
/(
    (?=
        (   coverage
          | test_
        )
    ).*[.]py
)/
'''
# Supposed to run `pylint` on multiple cores, but
# I've not had good experience with this...
# jobs = 0

[tool.pytest.ini_options]
junit_family = "legacy"
filterwarnings = [
    'error',
    'ignore::ResourceWarning',
    # bogus numpy ABI warning (see numpy/#432)'
    'ignore:.*numpy.dtype size changed.*:RuntimeWarning',
    'ignore:.*numpy.ufunc size changed.*:RuntimeWarning',
    # from usage of numpy_to_vtk:'
    'ignore:.*`np.bool` is a deprecated alias.*:DeprecationWarning',
    'ignore:.*`np.int` is a deprecated alias.*:DeprecationWarning',
    'ignore:.*`np.float` is a deprecated alias.*:DeprecationWarning',
    'ignore:.*`np.object` is a deprecated alias.*:DeprecationWarning',
    'ignore:.*`np.long` is a deprecated alias:DeprecationWarning',
    'ignore:.*Converting `np\.character` to a dtype is deprecated.*:DeprecationWarning'
]
# Some more Adam favorites...
# minversion = "7.0"
# addopts = """\
# --last-failed --last-failed-no-failures all \
# -p no:faulthandler \
# --import-mode=importlib \
# --randomly-seed=1234 \
# --cov \
# --cov-report term-missing \
# --cov-report html \
# --cov-report xml \
# --doctest-rst \
# --doctest-modules"""
# testpaths = [
#     "tests",
# ]
# qt_api = "pyside6"
# doctest_plus = "enabled"
# env = [
#     # "D:QT_QPA_PLATFORM=offscreen"
#     # "D:COVERAGE_DEBUG=trace",
#     # "D:COVERAGE_DEBUG_FILE=debug_log.txt"
# ]
# filterwarnings = [
#     # Occurs when mocking QWidgets
#     'ignore:pyside_type_init:RuntimeWarning'
# ]

And now for the main event...

I am assuming we are using the latest version of setuptools (i.e. >=60.10.0). From the setuptools docs:

zip_safe
A boolean (True or False) flag specifying whether the project can be safely installed and run from a zip file. If this argument is not supplied, the bdist_egg command will have to analyze all of your project’s contents for possible problems each time it builds an egg.

Excerpting from realpython.com:

Zip imports were introduced in PEP 273 and are helpful when you need to distribute a package as a single file (its most common use case). PEP 302 added import hooks that give you import from a zip file, so long as it is in python's module search path.

Using the builtin module zipfile class PyZipFile, you can bundle your code into ZIP files, but there are limits:

  • You can't load dynamic files from the zipped package: e.g. .pyd, .dll, .so, or C-Extensions modules (e.g. made via Cython). You can get around this by extracting the files, writing them to disk, and loading them, but you have to deal with security risks regarding that (hence the "safe" in "zip_safe")
  • It doesn't compress your modules and packages, but just stores them in a ZIP file container (you'd have to use Deflate, bzip2, or LZMA for that). Compressing source files hurts import time performance since they must be decompressed. Also, the user must have the zlib

Another limitation not discussed in the article is that your code base will have to be refactored to use importlib.resources to be able to import them resource files from the zip, as explained in Barry Warsaw's PyCon 2018 talk. UPDATE: e.g. consider icons, images, etc.

AFAIK, we do not distribute pyvista as a ZIP package, so this should not affect us to set zip_safe=False. Otherwise, we would have to make a Stub-only Package. @akaszynski @adeak Can you comment (or any other maintainer/collaborator who knows)?

Let's add this.

@akaszynski @adeak Yes, I will add this to the MANIFEST.in

…afe=False`

`mypy` requires packages built with `setuptools` use the `zip_safe=False` option so it can find the package and `*.pyi` and `*.typed` files be added to the `MANIFEST.in` so they are included in the `sdist`.

See https://mypy.readthedocs.io/en/latest/installed_packages.html#creating-pep-561-compatible-packages
@adam-grant-hendry
Copy link
Contributor Author

@akaszynski @adeak @tkoyama010 Regarding transferring package configuration values to pyproject.toml, I'd prefer to do that in a separate PR. I just wanted to point it out.

akaszynski
akaszynski previously approved these changes Jul 1, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

I’m happy with this.

I’m also fine with a follow up PR improving our pyproject.toml, if only for specifying setuptools as our build system.

tkoyama010
tkoyama010 previously approved these changes Jul 1, 2022
@adeak adeak dismissed stale reviews from tkoyama010 and akaszynski via f62328c July 1, 2022 08:12
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

I pushed a trivial cosmetic change (new line at end of MANIFEST.in). Consider the approvals intact.

@adam-grant-hendry
Copy link
Contributor Author

I pushed a trivial cosmetic change (new line at end of MANIFEST.in). Consider the approvals intact.

Newlines! Hahaha. Thank you for fixing!

@adam-grant-hendry
Copy link
Contributor Author

I’m happy with this.

I’m also fine with a follow up PR improving our pyproject.toml, if only for specifying setuptools as our build system.

Thank you @akaszynski ! If you and @tkoyama010 can approve and merge, I will work on the next PR to update the pyproject.toml accordingly.

@adeak
Copy link
Member

adeak commented Jul 1, 2022

On the pyvista repo we try to wait 24 hours after approval before merging in order to give everyone around the globe a chance to check PRs.

@adam-grant-hendry
Copy link
Contributor Author

On the pyvista repo we try to wait 24 hours after approval before merging in order to give everyone around the globe a chance to check PRs.

Oh I'm so sorry, I forgot! That was exactly what we brought up in our discussion Improving review habits #2245. My bad!

Sorry for jumping the gun, I apologize.

@adeak
Copy link
Member

adeak commented Jul 1, 2022

No worries at all, I just wanted to explain why we might seem like we're dragging our feet here :) And in any case with an 8-line diff here and 3 approvals across 3 continents this is probably fine.

@akaszynski akaszynski merged commit 43d388b into pyvista:main Jul 2, 2022
adeak added a commit to adeak/pyvista that referenced this pull request Jul 6, 2022
* upstream/main:
  Add setup and teardown functionality to plot_directive (pyvista#2907)
  Update hypothesis requirement from <6.48.4 to <6.49.2 (pyvista#2942)
  Bump lxml from 4.9.0 to 4.9.1 (pyvista#2936)
  Bump typing-extensions from 4.2.0 to 4.3.0 (pyvista#2939)
  Update hypothesis requirement from <6.48.3 to <6.48.4 (pyvista#2938)
  Bump trimesh from 3.12.6 to 3.12.7 (pyvista#2937)
  Update information of release (pyvista#2911)
  update pre-commit hooks (pyvista#2931)
  Handle invalid theme on init (pyvista#2917)
  Add `py.typed` Marker (pyvista#2904)
  [create-pull-request] update local intersphinx (pyvista#2926)
  Add tags with python commands to avoid misnumbering (pyvista#2416)
  Fix the url link of discretize (pyvista#2914)
  Add DataSet.cell_point_ids (pyvista#2897)
  Update hypothesis requirement from <6.48.2 to <6.48.3 (pyvista#2918)
  Ensure data remains a shallow copy when added to plotter (pyvista#2888)
@adam-grant-hendry adam-grant-hendry deleted the maint/pytyped branch July 8, 2022 16:34
@@ -0,0 +1 @@
partial\n
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a stub package distribution is partial it MUST include partial\n in a py.typed file.

I believe it needs a newline. We can remove the \n, but the PEP seems to identify the file must end with a new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve posted the question on the Python Discussion Forum:

https://discuss.python.org/t/pep-561-clarification-regarding-n/32875

My main question is whether the \n is deliberately required in order to distinguish Linux/MacOS line endings from Windows ones (\r\n).

git config core.autocrlf can be configured to store files with Linux line endings, but when checking out the repo on Windows, these are converted back to \r\n, which I wonder if type checkers will honor or if they only honor \n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eggplants Per the discussion, this works with or without the newline in the mypy implementation since that checker strips all whitespace, including explicit escape sequences (I have not looked at pyre, pyright, or other type checkers).

It seems a new line is requested to be included by convention, specifically the POSIX convention (ASIDE: A newline is useful in the event this file must be concatenated with another prior to processing).

This works with the \n (the package is registered as partially typed), so nothing is breaking.

It’s a “6 one way, half a dozen the other” problem, so if we want this changed, I recommend removing the escape sequence, but adding a new line to the end of the file (i.e. hit Enter/Return on your keyboard one more time).

Copy link
Member

Choose a reason for hiding this comment

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

@adam-grant-hendry I've been trying to make it clear that this shouldn't work. What we have in this PR doesn't include an escape sequence. The exact contents of the file are 'partial\\n\n'. There's a literal backslash, a literal n, and then an actual linefeed character. If you strip away this content you get 'partial\\n'. Odds are the typing "works" because of the "edge case" code path in the above mypy code (I haven't looked closely).

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not worked with mypy.

https://github.com/python/mypy/blob/ed9b8990025a81a12e32bec59f2f3bfab3d7c71b/mypy/modulefinder.py#L439C4-L439C4

fscache.read(stub_typed_file).decode().strip() returns 'partial\\n' and it is not recognized as Partial typed package because 'partial\\n' == 'partial' returns False.

Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to make it clear that this shouldn't work.

Then why are we “fixing” a non-problem?

I can't really explain it in any other way. Please reread my comments (also on discuss) carefully. What "shouldn't work" is the file contents we have now. If it works now it's accidental. We are "fixing it" because it's wrong. The literal backslash and literal n must go. @eggplants' suggestion here is the fix to make it correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fscache.read(stub_typed_file).decode().strip() returns 'partial\n' and it is not recognized as Partial typed package because 'partial\n' == 'partial' returns False.

Okay, THAT's the answer I was looking for. Thank you for the clear explanation. If that's the case, then certainly, partial\n does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eggplants Did you happen to open a separate PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just created! #4863

@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Sep 8, 2023

FWIW, I've submitted a PR to clarify PEP 561.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants