Simplify imports #594

jmfrank63 · 2018-08-07T15:41:56Z

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.

Zac-HD · 2018-08-08T04:42:24Z

This looks great! You can see the formatting issues Travis is complaining about here, and fix them by running

pip install yapf==0.20.1
yapf -rpi setup.py trio

After that, we'll need a test that every name in the __all__ list appears in the trio namespace, i.e.

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 __all__ works for pylint and mypy; and if not to declare it up front and write another test that it's consistent with the lower-level __all__s. However, that step can be left to a future pull request by someone else.

Finally - and most importantly - thanks for contributing! 😍 🎉

njsmith · 2018-08-08T05:18:39Z

trio/_version.py

@@ -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"


No need to change the version here :-) We'll do that when we release.

njsmith · 2018-08-08T05:21:00Z

If we're switching to explicitly listing all re-exports in the import statements... is there any point in maintaining the __all__ lists at all? Either in trio.__all__, or inside the individual sub-modules? Surely static analysis tools are all clever enough to understand that if they see from ... import X then that means X is defined, right...?

Zac-HD · 2018-08-08T05:45:16Z

@njsmith - at runtime, __all__ just defines which names are imported by from foo import *. However it's also widely used for static analysis (eg pylint and mypy), because PEP8 defines names which appear in __all__ as public API.

Since the goal of #542 was to improve the static analyse-ability of Trio, I'd like to keep or write __all__ literals for all our public modules. (this is actually the main blocker for static typing, FWIW)

njsmith · 2018-08-08T06:08:18Z

@Zac-HD Huh, the rule for import * is that if there's an __all__, it uses that, and if there isn't, it defaults to "all non-underscored names". I would have assumed that pylint/mypy/etc. knew that rule. Are you sure pylint etc. really treat these two modules differently?

# module 1
def f():
    pass

# module 2
def f()
    pass
__all__ = ["f"]

And if so, I'm super curious what the difference is...

Zac-HD · 2018-08-08T06:21:04Z

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 __all__ in e.g. pydocstyle, and having empty declarations is really useful for Sphinx autodoc and similar tools which walk namespaces looking for something to do.

dhirschfeld · 2018-08-08T06:35:17Z

trio/__init__.py

+                                        current_time,
+                                        TaskLocal,
+                                        __all__)
+


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, )

Yep, the compact form is enforced by yapf; see my comment above re: Travis.

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.

njsmith · 2018-08-08T07:28:54Z

@Zac-HD

In particular I've encountered a few weird issues with non-literal __all__ in e.g. pydocstyle

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 trio/__init__.py, I think we basically have these four options:

# 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 __all__ += _submodule.__all__ thing may or may not be understood by static analysis tools. Options C and D don't have that problem.

With B and C, the definition of __all__ is more annoying for us to maintain, because now every time we add a new export, we have to manually update both the import line and the definition of __all__, and make sure they stay in sync. (With B we have to manually update trio._submodule.__all__; with C we have to manually update trio.__all__, but either way it's two places that need updating.)

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 __all__ to confuse static analysis because there is no __all__, and the exports are only listed once, so maintenance is easier.

In fact I assume mypy has to mostly ignore __all__, because when it sees something like trio.open_nursery it has to track that back to a definition so it can find the type signature – and only the import line can help it do that.

Zac-HD · 2018-08-08T08:12:30Z

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.

njsmith · 2018-08-08T08:35:46Z

Either way, it would be good to write a test that everything in the lower-level modules __all__ is exported at the top level.

I'd be tempted to get rid of the lower-level __all__'s too. If we're not using them, then what's the point of maintaining and testing them?

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 __all__, but forgets to add it to the higher-level import line – AND if we didn't have the __all__ then they would have forgotten to export it anywhere. I guess this scenario could happen? We all forget stuff sometimes. But I feel like if someone's going to forget to export something, then it doesn't help to have two places they're supposed to export it.

jmfrank63 · 2018-08-08T08:52:15Z

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> .

dhirschfeld · 2018-08-08T10:22:02Z

I'm no doubt missing something but is there a reason you need to define an __all__ in __init__.py? If you do:

from ._submodule1 import *
from ._submodule2 import *

...that will only import the public objects (defined in __all__) from the submodules. I'm pretty sure that's the pattern I've used - define all at the submodule level, not in the __init__.py. Does that confuse static analysis tools? It's certainly possible but not something I've noticed.

Edit: I did a bit of testing to make sure I wasn't crazy. What I found was that importing * picked up everything - so importing something that wasn't in __all__ didn't show up as an error in static analysis. This appeared to be true using either jedi or the python language server in vscode. The only way to avoid false positives appeared to be explicit imports in the __init__.py. Ugh.

njsmith · 2018-08-08T16:50:36Z

@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 b is available in the __init__.py namespace?

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.

njsmith · 2018-08-08T16:59:30Z

@jmfrank63 I think first let's settle how we want to handle trio/__init__.py. Then we'll probably want to apply the same change to the other files that currently follow the same pattern:

trio/hazmat.py
trio/testing/__init__.py
trio/_core/__init__.py

For hazmat and _core this will be a little tricky because their exports are different on different platforms. But we can at least apply this trick to the parts that aren't platform-dependent.

And then after that, we'll want to think about what to do with the trio.abc and trio.socket modules, which are a bit different.

dhirschfeld · 2018-08-08T20:14:29Z

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.

jmfrank63 · 2018-08-09T10:19:14Z

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 · 2018-08-09T15:45:50Z

Codecov Report

Merging #594 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
trio/_deprecate.py	`100% <ø> (ø)`	⬆️
trio/_toplevel_core_reexports.py	`0% <ø> (-100%)`	⬇️
trio/_util.py	`100% <100%> (ø)`	⬆️
trio/tests/test_exports.py	`100% <100%> (ø)`	⬆️
trio/_signals.py	`100% <100%> (ø)`	⬆️
trio/__init__.py	`100% <100%> (ø)`	⬆️

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 8e16393...3afdb8b. Read the comment docs.

jmfrank63 · 2018-08-09T16:20:15Z

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.
I left __all__intact, but I moved the call to the globals into a function so we can import it and call it for the reexport. I thought this will not hurt but might be beneficial for the future.
I don't think it is called anyway now that we do an explicit import and not a *, but I could be wrong.
Pycharm still works with the git repro installed and shows all the completions vscode with the python extension shows.
Jedi vim is currently in progress, setup is tricky due to the lack of virtualenv picked up by jedi.

njsmith · 2018-08-09T17:26:37Z

@jmfrank63 Thanks for checking in with the pylint developers – that's super interesting to hear that they explicitly don't use __all__. (Link to the pylint issue: pylint-dev/pylint#2382 (comment). By the way, as a general tip, I like to link between issues like this as much as I can, because it helps when someone is trying to track down why a decision was made like 5 years later and they have no idea what's going on. I guess github has some official syntax for linking to issues in other repositories, but I can't be bothered to learn it – I just cut and paste the actual url into my comment, and then github notices that it's a github url and renders it nicely.)

For this PR:

It sounds like it's more or less settled now that __all__ isn't doing anything useful. Can you delete all the __all__ update lines in trio/__init__.py, and also in all the trio/_something.py files that the __init__.py file imports from? (Just those files, not any others – for example trio/_abc.py will probably need some changes eventually but for this PR we shouldn't change it.)
Regarding trio/_toplevel_core_reexports.py: this whole file only exists because of an earlier attempt to fix this same issue. I guess it helps with pycharm, but apparently not with any other analyzer (because all it does is try to make __all__ more visible). Our new strategy replaces it. So we should delete this file entirely, and in __init__.py we should write:

from ._core import (
   # ... everything we currently import from _toplevel_core_reexports
)

For writing a test: the basic idea is to open one of the files in the tests directory – for this I would use trio/tests/test_exports.py – and add a function named test_<whatever you like>. The function should assert that whatever it's testing is true. Then to run the tests, use pytest (there's some more details here: https://trio.readthedocs.io/en/latest/contributing.html#pull-request-tests). If you haven't done this before, I'd try just locally playing around with writing tests like assert 1 + 1 == 2 and assert 1 + 1 == 987 to get the hang of it.

Then for the actual test, I suppose we want to use the pylint and/or Jedi APIs to query for what exports they see, and compare that to what we see when looking at the same namespace dynamically. For an example of how to use the pylint API, see here. Jedi has an example of how to get completions on the first page of its docs; we could ask it for completions for a script like "import trio; trio.".

Please do ask for help if you get stuck!

jmfrank63 · 2018-08-13T10:26:26Z

The changes just as they are get a difference in jedi of

{'__builtins__', '__cached__', '__loader__', '__path__', '__spec__'}

and

{'fixup_module_metadata'}

with pylint.

njsmith · 2018-08-14T08:44:44Z

We could move the fixup_module_metadata stuff to a submodule, if it makes this easier to test. Or just import fixup_module_metadata as _fixup_module_metadata.

jmfrank63 · 2018-08-17T01:02:50Z

I wrote a first test against pylint:

def test_pylint_sees_all_names_in_namespace():
    # Test pylints ast to contain the same content as dir(trio)
    
    linter = PyLinter()
    ast_set = set(linter.get_ast('trio/__init__.py', 'trio'))
    trio_set = set([symbol for symbol in dir(trio) if symbol[0] != '_'])
    trio_set.remove('tests')
    assert trio_set - ast_set == set([])

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:

TESTING = False
if TESTING:
    from . import testing

So we could define an environment variable that if not present skips the import otherwise imports.
Any suggestions?

njsmith · 2018-08-17T05:28:19Z

I think it'd be fine for the test to special-case "testing" as a name that pylint/jedi might not pick up.

njsmith

A few quick comments I noticed while looking in...

njsmith · 2018-08-17T05:29:36Z

trio/__init__.py

+    TrioInternalError, RunFinishedError, WouldBlock, Cancelled,
+    ResourceBusyError, ClosedResourceError, MultiError, run, open_nursery,
+    open_cancel_scope, current_effective_deadline, TASK_STATUS_IGNORED,
+    current_time, TaskLocal, __all__


You shouldn't be importing __all__ here I think.

njsmith · 2018-08-17T05:30:05Z

trio/__init__.py

@@ -12,59 +12,62 @@

 from ._version import __version__

-__all__ = []
+from ._toplevel_core_reexports import (


I think we can make this from ._core import ..., and delete the _toplevel_core_reexports.py file entirely.

njsmith · 2018-08-17T10:26:01Z

trio/tests/test_exports.py

+
+def test_jedi_sees_all_completions():
+    # Test the jedi completion library get all in dir(trio)
+    script = jedi.Script(path=trio.__file__)


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.)

njsmith · 2018-08-17T10:27:23Z

trio/tests/test_exports.py

+    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')


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""?

Ok, I'll work myself through all the comments. Will take some time as this is all new stuff.

No worries, take your time, and don't be afraid to ask for help if you get stuck :-)

njsmith · 2018-08-19T03:08:35Z

@jmfrank63 Ah, so what's happened is you've discovered some places where we were unexpectedly relying on __all__. Exciting! :-)

In test_exports, it checks for symbol in source.__all__. Here symbol is the name of something exported from trio._core, like "TrioExternalError", and source is one of the modules that's supposed to re-export those symbols, like trio. But now we've gotten rid of trio.__all__, so this test no longer makes sense – we don't want "TrioExternalError" to show up in trio.__all__.

The quick fix is replace symbol in source.__all__ with hasattr(source, symbol). Later on when we clean up trio/_core/__init__.py we might also need to adjust the for symbol in _core.__all__ loop that's a few lines up, but for right now that's not necessary.

In test_module_metadata_is_fixed_up, the problem is an actual bug in fixup_module_metadata. Here's the source:

trio/trio/_util.py

Lines 171 to 182 in ffb5bca

    
           def fixup_module_metadata(module_name, namespace): 
        
               def fix_one(obj): 
        
                   mod = getattr(obj, "__module__", None) 
        
                   if mod is not None and mod.startswith("trio."): 
        
                       obj.__module__ = module_name 
        
                       if isinstance(obj, type): 
        
                           for attr_value in obj.__dict__.values(): 
        
                               fix_one(attr_value) 
        
               for objname in namespace["__all__"]: 
        
                   obj = namespace[objname] 
        
                   fix_one(obj)

Notice how on line 180, we've got another of those references to __all__. The goal of this function is to take a module namespace, go through all the objects it exports, and fix up their __module__ attribute so that when someone later tries to figure out the public name of this object, they get the right thing. For example, at the bottom of tracebacks it shows you the name of the exception type:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
trio.TooSlowError: timeout expired

Notice how it says trio.TooSlowError there? That trio is coming from the exception type's __module__ attribute. If we didn't rewrite it, then it would instead say trio._timeouts.TooSlowError: timeout expired, which exposes internal implementation details and is also kinda weird and distracting.

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 __all__, but of course that doesn't work once we stop using __all__ to control public exports. Instead, we can rewrite that loop like:

for objname, obj in namespace.items():
    if not objname.startswith("_"):  # ignore private attributes
        fix_one(obj)

jmfrank63 · 2018-08-19T20:32:52Z

Great, I'll try this and see how it works. Currently moving my home so some delay may occur.

njsmith · 2018-08-23T01:10:13Z

Filed a bug upstream with jedi to ask about the skipped test: davidhalter/parso#47

jmfrank63 · 2018-09-06T08:24:44Z

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> .

belm0 · 2018-09-07T06:06:54Z

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

dhirschfeld · 2018-09-07T23:16:04Z

test-requirements.txt

@@ -1,4 +1,4 @@
-pytest >= 3.3  # for catchlog fixture
+pytest == 3.7.4  # for catchlog fixture


Is there a particular reason for pinning to exactly 3.7.4 rather than just using >=?

This was just a workaround to see the real errors. See #650
The change has been reverted and is done in PR#650

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.

…xports.py

Merged with master to get CI to pass

njsmith · 2018-09-08T00:41:51Z

We don't need _toplevel_core_reexports.py anymore, right? it doesn't look like anything is using it now...

Zac-HD

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:):

The formatting nitpick I note below
deleting trio/_toplevel_core_reexports.py entirely (via @njsmith)
~~Refactoring namespace cleanup code in your excellent tests~~ (also via @njsmith), which I plan to copy for Hypothesis 😄 [deferred to a future PR]

Zac-HD · 2018-09-08T01:57:47Z

trio/_signals.py

@@ -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__()}


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.

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...

njsmith · 2018-09-08T05:02:04Z

@Zac-HD Thanks for reviewing! Re:

Refactoring namespace cleanup code in your excellent tests

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 :-)

belm0 · 2018-09-08T07:09:53Z

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

Zac-HD

This looks good to me - thanks again @jmfrank63 🎉

njsmith · 2018-09-08T09:39:37Z

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.

jmfrank63 · 2018-09-08T10:26:56Z

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> .

njsmith · 2018-09-08T19:48:04Z

@jmfrank63 For the mechanics, see: https://trio.readthedocs.io/en/latest/contributing.html#pull-request-release-notes

Basically: make a file called newsfragments/542.bugfix.rst (or feature? Idk, use your judgement :-)), and write a sentence or two to explain to users why they should be interested in upgrading to 0.8.0. For examples, you can use the previous release notes: https://trio.readthedocs.io/en/latest/history.html

jmfrank63 · 2018-09-09T14:50:46Z

Now, that the pull request is merged, how to add a newsfragment? Via an additional PR?

njsmith · 2018-09-09T17:46:24Z

@jmfrank63 yep, exactly

njsmith reviewed Aug 8, 2018

View reviewed changes

dhirschfeld reviewed Aug 8, 2018

View reviewed changes

njsmith mentioned this pull request Aug 9, 2018

pylint not finding members on trio library pylint-dev/pylint#2382

Closed

njsmith reviewed Aug 17, 2018

View reviewed changes

njsmith mentioned this pull request Aug 23, 2018

question: what's your timeline for supporting new python versions? davidhalter/parso#47

Closed

njsmith changed the base branch from master to release August 23, 2018 07:13

jmfrank63 added 5 commits September 7, 2018 22:02

Readded trio/_toplevel_core_reexports.py

e7d2fe0

Synced .gitignore

5fc025b

removed again TaskLocal from trio/__init__.py

d569f5e

reverted docs changes, removed TESTING variable and removed explicit …

79e177e

…list from _core/__init__.py

Changed jedi completion test script to be an explicit script

fe82149

dhirschfeld reviewed Sep 7, 2018

View reviewed changes

jmfrank63 force-pushed the simplify_imports branch from 2c39f91 to fe82149 Compare September 7, 2018 23:35

jmfrank63 added 3 commits September 8, 2018 00:48

added open_signal_receiver to trio/__init__.py and _toplevel_core_ree…

1520424

…xports.py

Fixed formatting of trio/_toplevel_core_reexports.py

04106c0

Merge branch 'master' into simplify_imports

fcc808d

Merged with master to get CI to pass

Zac-HD reviewed Sep 8, 2018

View reviewed changes

jmfrank63 added 2 commits September 8, 2018 09:17

Added extra space after curly brace to check what happens about yapf …

3afdb8b

…on ci complaining about formatting

Deleted trio/_toplevel_core_reexports.py

b492ebb

Zac-HD approved these changes Sep 8, 2018

View reviewed changes

Zac-HD merged commit 312b470 into python-trio:master Sep 8, 2018

Zac-HD mentioned this pull request Sep 9, 2018

Initial work towards adding type hints #656

Merged

jmfrank63 deleted the simplify_imports branch September 9, 2018 21:09

This was referenced Mar 30, 2023

verifytypes does not include imports from underscored modules microsoft/pyright#4869

Closed

add type tests for all exported objects #2623

Closed

Add pyright --verifytypes to CI & checks #2625

Closed

		@@ -1,4 +1,4 @@
		pytest >= 3.3 # for catchlog fixture
		pytest == 3.7.4 # for catchlog fixture

Simplify imports #594

Simplify imports #594

Conversation

jmfrank63 commented Aug 7, 2018 • edited Loading

Zac-HD commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

njsmith commented Aug 8, 2018

Zac-HD commented Aug 8, 2018

njsmith commented Aug 8, 2018

Zac-HD commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Aug 8, 2018

Zac-HD commented Aug 8, 2018

njsmith commented Aug 8, 2018

jmfrank63 commented Aug 8, 2018 via email

dhirschfeld commented Aug 8, 2018 • edited Loading

njsmith commented Aug 8, 2018 • edited Loading

njsmith commented Aug 8, 2018

dhirschfeld commented Aug 8, 2018

jmfrank63 commented Aug 9, 2018 via email

codecov bot commented Aug 9, 2018 • edited Loading

Codecov Report

jmfrank63 commented Aug 9, 2018

njsmith commented Aug 9, 2018 • edited Loading

jmfrank63 commented Aug 13, 2018 • edited Loading

njsmith commented Aug 14, 2018

jmfrank63 commented Aug 17, 2018 • edited Loading

njsmith commented Aug 17, 2018

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Aug 19, 2018

jmfrank63 commented Aug 19, 2018

njsmith commented Aug 23, 2018

jmfrank63 commented Sep 6, 2018 via email

belm0 commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Sep 8, 2018

Zac-HD left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Sep 8, 2018

belm0 commented Sep 8, 2018

Zac-HD left a comment

Choose a reason for hiding this comment

njsmith commented Sep 8, 2018 • edited Loading

jmfrank63 commented Sep 8, 2018 via email

njsmith commented Sep 8, 2018

jmfrank63 commented Sep 9, 2018

njsmith commented Sep 9, 2018

jmfrank63 commented Aug 7, 2018 •

edited

Loading

Zac-HD commented Aug 8, 2018 •

edited

Loading

dhirschfeld commented Aug 8, 2018 •

edited

Loading

njsmith commented Aug 8, 2018 •

edited

Loading

codecov bot commented Aug 9, 2018 •

edited

Loading

njsmith commented Aug 9, 2018 •

edited

Loading

jmfrank63 commented Aug 13, 2018 •

edited

Loading

jmfrank63 commented Aug 17, 2018 •

edited

Loading

Zac-HD left a comment •

edited

Loading

njsmith commented Sep 8, 2018 •

edited

Loading