-
-
Notifications
You must be signed in to change notification settings - Fork 330
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
Simplify imports #594
Simplify imports #594
Conversation
This looks great! You can see the formatting issues Travis is complaining about here, and fix them by running
After that, we'll need a test that every name in the def test_dunder_all():
for name in trio.__all__:
assert hasattr(trio, name) This will ensure that we keep the imports up to date when new names are added at lower levels. The optional last step would be to check that the dynamically-created Finally - and most importantly - thanks for contributing! 😍 🎉 |
trio/_version.py
Outdated
@@ -1,3 +1,3 @@ | |||
# This file is imported from __init__.py and exec'd from setup.py | |||
|
|||
__version__ = "0.5.0+dev" | |||
__version__ = "0.5.1+dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change the version here :-) We'll do that when we release.
If we're switching to explicitly listing all re-exports in the import statements... is there any point in maintaining the |
@njsmith - at runtime, Since the goal of #542 was to improve the static analyse-ability of Trio, I'd like to keep or write |
@Zac-HD Huh, the rule for # module 1
def f():
pass # module 2
def f()
pass
__all__ = ["f"] And if so, I'm super curious what the difference is... |
I don't know of any tools that would treat those differently, but module three has no public API, and the interpretation of module four may vary depending on the tool. # module 3
__all__ = []
def f()
pass # module 4
__all__ = []
def f()
pass
__all__.append('f') # or +=, or .extend, etc. In particular I've encountered a few weird issues with non-literal |
trio/__init__.py
Outdated
current_time, | ||
TaskLocal, | ||
__all__) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the trio style is but rather than have towers of arguments at various arbitrary columns I personally prefer to drop down to the next line and only indent one level:
from ._toplevel_core_reexports import (
ClosedResourceError,
MultiError,
ResourceBusyError,
RunFinishedError,
TrioInternalError,
Cancelled,
WouldBlock,
TaskLocal,
TASK_STATUS_IGNORED,
current_effective_deadline,
current_time,
open_cancel_scope,
open_nursery,
run,
)
...this also allows for a more compact form:
from ._toplevel_core_reexports import (
ClosedResourceError, MultiError, ResourceBusyError, RunFinishedError,
TrioInternalError, Cancelled, WouldBlock, TaskLocal, TASK_STATUS_IGNORED,
current_effective_deadline, current_time, open_cancel_scope, open_nursery,
run,
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the compact form is enforced by yapf
; see my comment above re: Travis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yapf allows either of the two forms @dhirschfeld showed – IIRC you get the first one if you put a trailing comma on the last item, and the "more compact" one if you don't use a trailing comma.
Right, that's one of the motivations here (and see also #314, which has some commentary from one of the PyCharm devs on what it can/can't understand). Let me try phrasing my question a different way. For # option A
from ._submodule1 import *
__all__ += _submodule1.__all__
from ._submodule2 import *
__all__ += _submodule2.__all__
# option B
from ._submodule1 import X, Y
__all__ += _submodule1.__all__
from ._submodule2 import Z
__all__ += _submodule2.__all__
# Option C
from ._submodule1 import X, Y
from ._submodule2 import Z
__all__ = ["X", "Y", "Z"]
# option D
from ._submodule1 import X, Y
from ._submodule2 import Z
# (__all__ is left undefined) Trio master currently uses option A, and that hasn't worked out so well. So now we're picking between B, C, or D. With B, the With B and C, the definition of Option D, it seems to me, doesn't have either problem: the exports are clearly visible to static analysis tools, there's no chance for In fact I assume mypy has to mostly ignore |
I'd be happy with either C or D and leaning towards D as the less-effort option. Either way, it would be good to write a test that everything in the lower-level modules |
I'd be tempted to get rid of the lower-level I guess the case where this test would catch a bug is: someone adds a new symbol, and remembers to add it to the module's |
Hello Zac, Nathaniel,
thanks for accepting me as a contributor. I am still a freshman, so I will
follow your recommendations:
Use yapf and fix the formating (I used the parenthesis because of PEP 328.
I think we should keep this, but put more imports on each line not
exceeding col 80)
Add a test (I wonder if this will still work with Option D in place)
Replace all * imports with explicit versions. (Should I confine this only
to __init__.py or cover all modules like (abc.py e.g))
Revert the version string to the release version
Is there some mechanism I can mark this as a dev branch of my own (besides
the git branch). I used this to make sure I really was on my branch and it
helped me to distinguish the two.
Thanks
Johannes
…On Wed, Aug 8, 2018 at 9:12 AM Zac Hatfield-Dodds ***@***.***> wrote:
I'd be happy with either C or D and leaning towards D as the less-effort
option.
Either way, it would be good to write a test that everything in the
lower-level modules __all__ is exported at the top level.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#594 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8oHhe_T52MXlV-tOycp3kG8P4O2hAjks5uOp1wgaJpZM4VyaL2>
.
|
I'm no doubt missing something but is there a reason you need to define an from ._submodule1 import *
from ._submodule2 import * ...that will only import the public objects (defined in Edit: I did a bit of testing to make sure I wasn't crazy. What I found was that importing |
@dhirschfeld This is one of the frustrating things about this yeah – it's really hard to know what works and what doesn't :-(. To make sure I understand correctly: you're saying that if you write: # submodule.py
# has a mix of public and private with literal __all__
a = 1
b = 2
__all__ = ["a"]
# __init__.py
from .submodule import * ... Then both Jedi and VSCode think that now Before I saw your edit I was going to say well, maybe that's the one case that works and we can use it for the simpler parts of trio where that pattern applies, but if even that doesn't work then never mind. |
@jmfrank63 I think first let's settle how we want to handle
For And then after that, we'll want to think about what to do with the |
Yep, that appears to be the case - i.e. I wonder if PyCharm does any better... might check that out. |
Hi everyone,
since I created an issue with pylint, this is what I got. It fits with what
Davide found so I forward it here to everyone interested. I might look into
creating a brains file as mentioned after having the imports done.
@jmfrank63 <https://github.com/jmfrank63> Thanks for creating an issue!
So pylint stopped looking a couple of years ago at __all__. The problem was
like this example, folks tended to modify __all__ dynamically which
resulted in false positives in pylint due to it not being able to follow
all the paths for inferring what potential values might be added to that
list. With that being said, pylint is still able to find all your members,
just that it doesn't restrict itself to finding just those from __all__.
In trio's example the problem is not the dynamically built __all__, but the
fact that run is added using globals() which is something that pylint cannot
make sense of:
https://github.com/python-trio/trio/blob/master/trio/_toplevel_core_reexports.py#L34
The way we usually solve these issues is by adding a brain tip to astroid,
pylint's inference engine:
https://github.com/PyCQA/astroid/tree/master/astroid/brain. brain tips are
just a form of patching an AST so that pylint can be able to find a member
or another. Here's a simple one for example, where we just build an AST for
the collections module and return it:
https://github.com/PyCQA/astroid/blob/master/astroid/brain/brain_collections.py#L67
With that being said, the best persons to add those brain tips is for the
folks actually using that library :) So if you have some time, would you
mind sending a rough patch that adds trio there?
Cheers
Johannes
…On Wed, Aug 8, 2018 at 9:14 PM Dave Hirschfeld ***@***.***> wrote:
Then both Jedi and VSCode think that now b is available in the *init*.py
namespace?
Yep, that appears to be the case - i.e. val = mymodule.b didn't show as
an error using static analysis and b was a tab-completion option. In a
python terminal with live objects b correctly didn't show up as an
option. I was surprised by that, so much so that I'd call it a bug in
static analysis!
I wonder if PyCharm does any better... might check that out.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#594 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8oHgQM1IOHKS2rvKdu3Ig3IIK9gtKiks5uO0aogaJpZM4VyaL2>
.
|
Codecov Report
@@ Coverage Diff @@
## master #594 +/- ##
==========================================
- Coverage 99.31% 99.29% -0.03%
==========================================
Files 95 95
Lines 11028 11033 +5
Branches 786 791 +5
==========================================
+ Hits 10953 10955 +2
- Misses 56 59 +3
Partials 19 19
Continue to review full report at Codecov.
|
Hi I did the formatting and reverted the version. Currently there is not a test for this. I will need some guidance on the tests, as testing is an absolute weak spot with me. |
@jmfrank63 Thanks for checking in with the pylint developers – that's super interesting to hear that they explicitly don't use For this PR:
from ._core import (
# ... everything we currently import from _toplevel_core_reexports
)
Please do ask for help if you get stuck! |
The changes just as they are get a difference in jedi of
and
with pylint. |
We could move the |
I wrote a first test against pylint:
However since testing is not included by default (commented out) the test fails on the testing module not being in the namespace. But this one passes the test:
So we could define an environment variable that if not present skips the import otherwise imports. |
I think it'd be fine for the test to special-case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few quick comments I noticed while looking in...
trio/__init__.py
Outdated
TrioInternalError, RunFinishedError, WouldBlock, Cancelled, | ||
ResourceBusyError, ClosedResourceError, MultiError, run, open_nursery, | ||
open_cancel_scope, current_effective_deadline, TASK_STATUS_IGNORED, | ||
current_time, TaskLocal, __all__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't be importing __all__
here I think.
trio/__init__.py
Outdated
@@ -12,59 +12,62 @@ | |||
|
|||
from ._version import __version__ | |||
|
|||
__all__ = [] | |||
from ._toplevel_core_reexports import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make this from ._core import ...
, and delete the _toplevel_core_reexports.py
file entirely.
trio/tests/test_exports.py
Outdated
|
||
def test_jedi_sees_all_completions(): | ||
# Test the jedi completion library get all in dir(trio) | ||
script = jedi.Script(path=trio.__file__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way jedi.Script
is supposed to be used is maybe more like jedi.Script("import trio\ntrio.")
? (As in, we imagine somebody typed those characters into a file and then hit tab to get a list of completions.)
linter = PyLinter() | ||
ast_set = set(linter.get_ast(trio.__file__, 'trio')) | ||
trio_set = set([symbol for symbol in dir(trio) if symbol[0] != '_']) | ||
trio_set.remove('tests') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worthwhile to factor out some of the namespace cleanup code from the two functions – I think in both cases we want to test something like: "the two sets are equivalent, if you ignore names that .startswith("_")
, "tests"
, and "testing"
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll work myself through all the comments. Will take some time as this is all new stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, take your time, and don't be afraid to ask for help if you get stuck :-)
@jmfrank63 Ah, so what's happened is you've discovered some places where we were unexpectedly relying on In The quick fix is replace In Lines 171 to 182 in ffb5bca
Notice how on line 180, we've got another of those references to
Notice how it says Anyway, those details don't really matter right now, just giving you some context :-). The specific issue is that the function needs to iterate over all the public exports in a namespace, and it's trying to do that using for objname, obj in namespace.items():
if not objname.startswith("_"): # ignore private attributes
fix_one(obj) |
Great, I'll try this and see how it works. Currently moving my home so some delay may occur. |
Filed a bug upstream with jedi to ask about the skipped test: davidhalter/parso#47 |
Hi Nathaniel,
yes, I had been moving to a new home in the past two weeks and this has
proven to be far more disruptive than I imagined. Additionally I am in the
process of internally applying to a new role inside my company which drew
additional resources. I'll be back on this on the weekend.
I already learned so much about git and the development process, I won't
give up on this.
Sorry for the delay, and thanks for asking.
Cheers
Johannes
…On Thu, Sep 6, 2018 at 6:29 AM Nathaniel J. Smith ***@***.***> wrote:
Hi @jmfrank63 <https://github.com/jmfrank63> – are you still working on
this? (No worries if not – we figured out a lot about what needs to be done
that someone else could pick up from here, and there are plenty of other
things to do if you want to get involved again later :-).)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#594 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8oHkDzpcWY3kPNtSrrgKczOvyjFBVSks5uYLLGgaJpZM4VyaL2>
.
|
I think this is the top user-happiness issue of Trio currently (watchdog seems like a distant 2nd). @jmfrank63 do you have an idea of time frame? I'm happy to take a stab this weekend otherwise. |
…list from _core/__init__.py
test-requirements.txt
Outdated
@@ -1,4 +1,4 @@ | |||
pytest >= 3.3 # for catchlog fixture | |||
pytest == 3.7.4 # for catchlog fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason for pinning to exactly 3.7.4 rather than just using >=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a workaround to see the real errors. See #650
The change has been reverted and is done in PR#650
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! When making a change such as pinning to a particular version it's useful to users to make a comment (in the commit message) as to why so they know whether or not they can safely ignore it e.g. because the underlying issue has been fixed.
Fixing it in #650 is great because now the blame https://github.com/jmfrank63/trio/blame/528a2986434d644be8138219f0f3087e111d79e8/test-requirements.txt#L1 is linked to the PR and so a user can follow that all the way to #651 to see if the problem has been fixed and they're safe to use pytest 3.8.1 with trio.
2c39f91
to
fe82149
Compare
Merged with master to get CI to pass
We don't need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for taking this on @jmfrank63 - I know it's a much larger job than it appears, but it's also appreciated by more people than will comment here 😍
I think that there are only three things left before we can merge this (:tada:):
trio/_signals.py
Outdated
@@ -177,7 +177,7 @@ def __aiter__(self): | |||
return self | |||
|
|||
async def __anext__(self): | |||
return { await self._signal_queue.__anext__()} | |||
return {await self._signal_queue.__anext__()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line causing Travis to fail - yapf
requires that leading space. I'd just drop it from the diff, as it's not really related to the imports issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is some weird thing where yapf is apparently doing different for @jmfrank63 than for the rest of us, even with the same version and settings. ¯\(ツ)/¯
And yeah, simplest thing is to manually delete the space...
@Zac-HD Thanks for reviewing! Re:
I think I'd also be OK with putting this off to a follow-up PR... this one has been dragging on a long time and we don't want it to get stuck again :-) |
My project just started using pylint for static checking in the continuous build, and many on our team rely on the PyCharm IDE, so this change will be fantastic-- thank you! For the sake of those looking through historical commits, I'd like to ask again that the "squash" option be used on merge since 50 commits is a lot for what's ultimately a relatively small change. |
…on ci complaining about formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me - thanks again @jmfrank63 🎉
You know, in all that, I think we forgot to add a newsfragment. We should do that so users will realize how hard we're working. |
New to this one, too. It’s probably just a sentence or two, is there any
guideline or best practice?
…On Sat, 8 Sep 2018 at 10:39, Nathaniel J. Smith ***@***.***> wrote:
You know, in all that, I think we forgot to add a newsfragment. We should
do that so is will realize how hard we're working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#594 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8oHhr0n0TTVLo1Osv7Rdh8UT4DKD4cks5uY5BbgaJpZM4VyaL2>
.
|
@jmfrank63 For the mechanics, see: https://trio.readthedocs.io/en/latest/contributing.html#pull-request-release-notes Basically: make a file called |
Now, that the pull request is merged, how to add a newsfragment? Via an additional PR? |
@jmfrank63 yep, exactly |
As advised I replaced all import * with explicit imports in
__init__.py
of the trio root.I tested against vscode code completion and it seems to work. I did not run any tests yet.
Also pylint does not complain anymore about missing attributes.
The issue was #542.