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

👌 IMPROVE: Use __all__ to signal re-exports #120

Merged
merged 11 commits into from
Dec 1, 2021

Conversation

hukkin
Copy link
Member

@hukkin hukkin commented Jan 12, 2021

No description provided.

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #120 (3b560f5) into master (22a65b3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   96.08%   96.08%           
=======================================
  Files          60       60           
  Lines        3242     3248    +6     
=======================================
+ Hits         3115     3121    +6     
  Misses        127      127           
Flag Coverage Δ
pytests 96.08% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
markdown_it/__init__.py 100.00% <100.00%> (ø)
markdown_it/helpers/__init__.py 100.00% <100.00%> (ø)
markdown_it/presets/__init__.py 100.00% <100.00%> (ø)
markdown_it/rules_block/__init__.py 100.00% <100.00%> (ø)
markdown_it/rules_core/__init__.py 100.00% <100.00%> (ø)
markdown_it/rules_inline/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22a65b3...3b560f5. Read the comment docs.

@hukkin hukkin changed the title 👌 IMPROVE: Use __all__ to signal that MarkdownIt is importable from root package 👌 IMPROVE: Use __all__ to signal re-exports Jan 12, 2021
@hukkin
Copy link
Member Author

hukkin commented Jul 7, 2021

@chrisjsewell To clarify why this is useful: this allows downstream projects (and markdown-it-py itself) that use the packaged type data to set implicit_reexport = False mypy flag. This flag will protect from accidental bad imports by making them mypy errors. Here's examples of bad imports that unfortunately do work at runtime but are not public API and should never be used:

from markdown_it.token import attr
from markdown_it.tree import Token

If you dislike setting the __all__ attribute, an alternative way to express explicit re-export that mypy understands is an import using "as":

from .state_core import StateCore as StateCore

@chrisjsewell
Copy link
Member

cheers, I think this is fine, one nitpick would be I think the __all__ should be underneath the imports, unless you no of a reason why not?
and then can you also look at fixing the documentation build fail

@hukkin
Copy link
Member Author

hukkin commented Aug 17, 2021

one nitpick would be I think the all should be underneath the imports, unless you no of a reason why not?

This is simply what PEP8 tells us to do so following that.

Hmm, I'll have a look at the docs failure in a short while.

@hukkin
Copy link
Member Author

hukkin commented Aug 17, 2021

The issue seems to be as follows:

>>> from markdown_it.rules_block.blockquote import blockquote
>>> repr(blockquote)
Out[3]: '<function blockquote at 0x7f7ebe68aaf0>'
>>> from markdown_it.rules_block import blockquote
>>> repr(blockquote)
Out[5]: '<function blockquote at 0x7f7ebe68aaf0>'

The name markdown_it.rules_block.blockquote is overloaded. It is both a module to import from and also an importable function. Sphinx autodocs do not seem to like this very much.

I suggest renaming the markdown_it.rules_block.blockquote module as markdown_it.rules_block._blockquote (i.e. with an underscore). That fixes the error and is in line with what's importable in JS markdown-it. The same fix would obviously be applied to all modules where module names follows the pattern markdown_it.rules_*.*.

Another way is to just ignore the Sphinx warnings.

What do you think?

@chrisjsewell
Copy link
Member

Hmm, I'll have a look at the docs failure in a short while.

Sorted it 😄

I suggest renaming the markdown_it.rules_block.blockquote module as markdown_it.rules_block._blockquote (i.e. with an underscore). That fixes the error and is in line with what's importable in JS markdown-it.

This is possibly the better solution eventually, but err I'd want to think about it some more, and so don't want to hold up this PR

@chrisjsewell chrisjsewell merged commit 73763b3 into executablebooks:master Dec 1, 2021
@hukkin hukkin deleted the all branch December 1, 2021 17:55
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