Switch to using standalone Outcome library. #503

Fuyukai · 2018-04-17T23:55:35Z

Closes #494. Closes #505.

Not sure what to do about the Result object section in hazmat docs yet.

codecov · 2018-04-18T00:04:37Z

Codecov Report

Merging #503 into master will decrease coverage by <.01%.
The diff coverage is 98.27%.

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   99.27%   99.26%   -0.01%     
==========================================
  Files          89       89              
  Lines       10418    10405      -13     
  Branches      720      721       +1     
==========================================
- Hits        10342    10329      -13     
  Misses         58       58              
  Partials       18       18

Impacted Files	Coverage Δ
trio/_core/_traps.py	`100% <ø> (ø)`	⬆️
trio/hazmat.py	`100% <ø> (ø)`	⬆️
trio/_core/__init__.py	`100% <ø> (ø)`	⬆️
trio/_core/_io_kqueue.py	`78.48% <100%> (+0.27%)`	⬆️
trio/_core/tests/test_run.py	`100% <100%> (ø)`	⬆️
trio/_core/tests/test_local.py	`100% <100%> (ø)`	⬆️
trio/_core/_result.py	`100% <100%> (ø)`	⬆️
trio/_core/_run.py	`100% <100%> (ø)`	⬆️
trio/_threads.py	`100% <100%> (ø)`	⬆️
trio/tests/test_timeouts.py	`100% <100%> (ø)`	⬆️
... and 6 more

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 94d49f9...0cbed90. Read the comment docs.

oremanj · 2018-04-18T01:33:12Z

trio/_core/_result.py


-    """
+class Result(outcome.Outcome):
+    @deprecated(version="0.5.0", issue=494, instead="outcome.Outcome")


Result was previously an ABC, so I wouldn't expect anyone to be trying to construct it; I think the deprecation notice would need to go on capture(), acapture(), and the constructors of Value and Error in order to have any effect.

For Value and Error I'd suggest using trio._deprecate's trick for module level deprecations. It lets you say "when someone accesses trio.hazmat.Value, then give them [this] deprecation message and then return [this] object." It looks like the diff for 089c6ff has an example.

It might even be simplest to use this for Result too. The advantage is that it gives a warning for isinstance(x, trio.hazmat.Result).

njsmith · 2018-04-18T02:03:32Z

Not sure what to do about the Result object section in hazmat docs yet.

You can delete the documentation of the Result objects themselves, and update the docs as if we always used outcome. (So e.g. wait_task_rescheduled's docstring will need some changes.)

Fuyukai · 2018-04-18T12:27:17Z

So, this conf.py nitpicky thing isn't very sustainable since we have to add every deprecated item there as it's in the changelog by a reference still.

njsmith · 2018-04-18T20:32:42Z

I think it would also be fine to convert the old changelog links into regular literal strings, if that's easier? (I know it feels weird to edit old changelogs, but it's just a formatting tweak, not a content change.) Unfortunately without the nitpicky thing it's very very easy to have broken links all over the place :-/.

smurfix · 2018-04-18T20:34:50Z

+1 on converting old changelog entries to literals. The link is dead, no sense in keeping it.

Fuyukai · 2018-04-18T22:05:31Z

Shoutouts to jenkins for arbitrarily refusing to install dependencies sometimes

Fuyukai · 2018-04-18T22:49:21Z

The build is failing because Travis hates me, I guess. Other than that, is all ready.

njsmith · 2018-04-19T03:44:06Z

The build failure was sphinx trying to fetch the intersphinx inventories, and then failing with the gnomic error message:

Warning, treated as error:
failed to reach any of the inventories with the following issues:

Seems like some weird transient thing... I hope? I just restarted the job anyway.

njsmith · 2018-04-19T03:45:50Z

Hmm. It failed again, while loading intersphinx inventory from https://outcome.readthedocs.org/objects.inv..., and indeed if I go to that URL I get a 404...

Maybe if you make the intersphinx link be https://outcome.readthedocs.io/en/latest/ it'll work better?

njsmith · 2018-04-19T06:23:55Z

Also, @Fuyukai, @smurfix, can you figure out how #505 and this should be coordinated? outcome just merged python-trio/outcome#11, so if we merge this, and then release outcome, without first dealing with #505, then I think trio will be broken?

njsmith · 2018-04-19T06:40:36Z

trio/_core/_result.py

+        _deprecate.DeprecatedAttribute(
+            Error, "0.5.0", issue=494, instead="outcome.Error"
+        )
+}


Sorry, this __deprecated_attributes__ thing is pretty confusing, we need some better docs here...

Since it's based on module attribute lookup, using this inside trio._core._result means that we'll issue warnings when someone mentions trio._core._result.<whatever>, which isn't really what we want – it's references to trio.hazmat.<whatever> that people might doing and we want to deprecate. So we want to apply the __deprecated_attributes__ stuff to trio.hazmat. In the version we used to have here we did that in trio/__init__.py, which seems as good as anything: 089c6ff#diff-9bcb0dac3a23179589f66a8646fa38faL107

Also, this isn't really relevant because of the previous issue, but as an FYI: I think this also wouldn't work because the __deprecated_attributes__ stuff is only checked if there isn't a real variable of the same name. (This is an optimization to avoid slowdowns on accessing non-deprecated attributes; it works using __getattr__.) So here where are actual variables named Value, Error, Result in the same module, it makes __deprecated_attributes__ a no-op.

njsmith · 2018-04-19T06:41:54Z

So having read over the diff, I think the complete todo list is:

intersphinx url
fixing __deprecated_attributes__
figuring out the plan to handle Trio fails when using single-use Outcome #505

Fuyukai · 2018-04-20T09:44:38Z

#505 was caused by The Curse of the Mutable Default. reschedule() had a default value for next_send of Value(None), which was being passed if there was no send - and promptly being unwrapped on the very first run loop. Then, when it was being yielded back up later on, it would be unwrapped again, and would explode.

…ry anyway)

Fuyukai · 2018-04-20T10:31:24Z

Ready to merge.

smurfix · 2018-04-20T10:37:12Z

trio/_core/_result.py

-            Error, "0.5.0", issue=494, instead="outcome.Error"
-        )
-}
+Result.register(Error)


Ugh. Could you please keep the newline?

Whoops, missed that. Surprised yapf didn't catch it.

njsmith · 2018-04-21T06:04:17Z

I filed #508 for the annoying jenkins failures, and once #509 is merged it might help us track them down.

- The dict needs to live in the module where the deprecated attributes are; so here it needs to be hazmat.__deprecated_attributes__. (Maybe it would be less confusing to put this in hazmat.py, I don't know.) - Don't export the symbols from trio.hazmat using normal mechanisms; that overrides the __deprecated_attributes__ mechanism. - Then since they're no longer in hazmat, we can't say that trio.hazmat.Value resolves to the value trio.hazmat.Value, so point the __deprecated_attributes__ entries directly at trio._core._result.* - In fact, let's stop exporting them from trio._core entirely. - And then this reveals two tests that were still referring to trio._core.Error, so fix those. Confirmed manually that with these changes I get correct values and warnings for accessing the public attributes: In [2]: trio.hazmat.Result /home/njs/.user-python3.5-64bit/bin/ipython:1: TrioDeprecationWarning: trio.hazmat.Result is deprecated since Trio 0.5.0; use outcome.Outcome instead (#494) #!/home/njs/.user-python3.5-64bit/bin/python3.5 Out[2]: trio._core._result.Result # class In [3]: trio.hazmat.Value /home/njs/.user-python3.5-64bit/bin/ipython:1: TrioDeprecationWarning: trio.hazmat.Value is deprecated since Trio 0.5.0; use outcome.Value instead (#494) #!/home/njs/.user-python3.5-64bit/bin/python3.5 Out[3]: outcome.Value # class In [4]: trio.hazmat.Error /home/njs/.user-python3.5-64bit/bin/ipython:1: TrioDeprecationWarning: trio.hazmat.Error is deprecated since Trio 0.5.0; use outcome.Error instead (#494) #!/home/njs/.user-python3.5-64bit/bin/python3.5 Out[4]: outcome.Error # class

njsmith · 2018-04-21T06:23:09Z

Just made some small fixups – in particular, the deprecated attributes stuff was still a bit mixed up (see the commit message for ed0510a). LGTM now – @Fuyukai, want to review my changes and then hit merge if you like them?

(At least we got lucky on Jenkins this time...)

oremanj reviewed Apr 18, 2018

View reviewed changes

Fuyukai added 4 commits April 18, 2018 23:06

Switch to using standalone Outcome library.

e27b50a

Rip out Result code from the rest of the core.

d645b06

Remove certain references in old changelog entries.

c7f4c67

Add outcome to intersphinx mapping

04452e5

njsmith reviewed Apr 19, 2018

View reviewed changes

Fuyukai added 3 commits April 20, 2018 10:35

Move deprecated attributes to the right place.

005b633

Fix RTD link

6587bbf

Solve the curse of the mutable default

cad04ae

Fuyukai added 3 commits April 20, 2018 10:44

Fix reschedule docstring

89f3150

Remove broken test code (this was basically testing a different libra…

d49962c

…ry anyway)

Fix some usages of _core.Value/Error and default Value values.

7bc2a18

smurfix reviewed Apr 20, 2018

View reviewed changes

Missing newline

768fdcf

njsmith added 2 commits April 20, 2018 23:07

Simplify socket_ready helper a bit

58a431a

Remove vestigial enable_attribute_deprecations call

83fec3f

Re-YAPFify

0cbed90

Fuyukai merged commit c31b946 into python-trio:master Apr 21, 2018

Fuyukai deleted the use-outcome branch May 5, 2018 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using standalone Outcome library. #503

Switch to using standalone Outcome library. #503

Fuyukai commented Apr 17, 2018 •

edited

Loading

codecov bot commented Apr 18, 2018 •

edited

Loading

oremanj Apr 18, 2018

njsmith Apr 18, 2018

njsmith commented Apr 18, 2018

Fuyukai commented Apr 18, 2018 •

edited

Loading

njsmith commented Apr 18, 2018

smurfix commented Apr 18, 2018

Fuyukai commented Apr 18, 2018

Fuyukai commented Apr 18, 2018

njsmith commented Apr 19, 2018

njsmith commented Apr 19, 2018

njsmith commented Apr 19, 2018

njsmith Apr 19, 2018

njsmith commented Apr 19, 2018

Fuyukai commented Apr 20, 2018

Fuyukai commented Apr 20, 2018

smurfix Apr 20, 2018

Fuyukai Apr 20, 2018

njsmith commented Apr 21, 2018

njsmith commented Apr 21, 2018

Switch to using standalone Outcome library. #503

Switch to using standalone Outcome library. #503

Conversation

Fuyukai commented Apr 17, 2018 • edited Loading

codecov bot commented Apr 18, 2018 • edited Loading

Codecov Report

oremanj Apr 18, 2018

Choose a reason for hiding this comment

njsmith Apr 18, 2018

Choose a reason for hiding this comment

njsmith commented Apr 18, 2018

Fuyukai commented Apr 18, 2018 • edited Loading

njsmith commented Apr 18, 2018

smurfix commented Apr 18, 2018

Fuyukai commented Apr 18, 2018

Fuyukai commented Apr 18, 2018

njsmith commented Apr 19, 2018

njsmith commented Apr 19, 2018

njsmith commented Apr 19, 2018

njsmith Apr 19, 2018

Choose a reason for hiding this comment

njsmith commented Apr 19, 2018

Fuyukai commented Apr 20, 2018

Fuyukai commented Apr 20, 2018

smurfix Apr 20, 2018

Choose a reason for hiding this comment

Fuyukai Apr 20, 2018

Choose a reason for hiding this comment

njsmith commented Apr 21, 2018

njsmith commented Apr 21, 2018

Fuyukai commented Apr 17, 2018 •

edited

Loading

codecov bot commented Apr 18, 2018 •

edited

Loading

Fuyukai commented Apr 18, 2018 •

edited

Loading