Opened 5 years ago

Closed 5 years ago

#4194 defect closed fixed (fixed)

Several broken API links in the core howto

Reported by: jesstess Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, thijs Branch: branches/bad-api-links-4194
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description

Including but not limited to:

process.xhtml:

  • reactor.spawnProcess()
  • reactor.run()
  • reactor.callWhenRunning()

threading.xhtml:

  • threads.blockingCallFromThread()

Change History (16)

comment:1 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/bad-api-links-4194

(In [27900]) Branching to 'bad-api-links-4194'

comment:2 Changed 5 years ago by jesstess

(In [27901]) Fix several broken API links in the core howto and comment out XXX
todos.

refs #4194.

comment:3 follow-ups: Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Some of the links, eg reactor.run, don't have corresponding API docs that would make for very clear link names and were converted to plain <code></code> blocks. Some had '()' at the end. Many of the PB links were to twisted.spread.flavors classes when the API docs are going to be for twisted.spread.pb.

I also commented out XXX comments. I don't feel strongly about making them go away but I don't think they inspire faith in new users.

It's hard to verify that the new links are correct beyond eyeballing it, so a second pass by someone would be great.

comment:4 Changed 5 years ago by khorn

application.html

  • It looks like TimerService actually does have a page in the API docs, so there's no reason to remove this one, unless you want to just for consistency with the other items in the list.

pb-copyable.html

  • Around line 570, it's not clear to me why you removed the API class from the link to twisted.spread.flavors. The page does actually exist, though of course pb.Cachable is not listed on the page.


In fact the whole twisted.spread.flavors vs. twisted.spread.pb situation
is kind of a mess, documentation-wise. The docs talk about flavors, but
none of the things being talked about are publicly exposed from that
module.


Not sure what (if anything) can/should be done about this.


comment:5 Changed 5 years ago by khorn

  • Keywords review removed
  • Owner set to jesstess

Doh, hit submit too soon...

Other than the above, the rest looks good to me.

comment:6 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Thanks for the review, khorn!

The pattern matching got ahead of me - I reverted the removal of links to TimerService in application.xhtml and twisted.spread.flavors in pb-copyable.xhtml.

comment:7 in reply to: ↑ 3 Changed 5 years ago by glyph

Replying to jesstess:

Some of the links, eg reactor.run, don't have corresponding API docs that would make for very clear link names and were converted to plain <code></code> blocks.

Wouldn't IReactorCore.run be the right thing to link to in that case?

comment:8 Changed 5 years ago by jesstess

(In [27919]) Add in a link to twisted.internet.interfaces.IReactorCore.run for
reactor.run in process.xhtml.

refs #4194

comment:9 follow-up: Changed 5 years ago by jesstess

The reason I'd switched reactor.run to a plain <code> block was because the surrounding material keeps referencing reactor.fooMethod and the code uses reactor.run, and I thought a link to IReactorCore.run would be less clear.

How is this as a compromise:

reactor.<code base="twisted.internet.interfaces.IReactorCore" class="API">run</code>

so the namespace is consistent but only the "run" is a hyperlink?

comment:10 Changed 5 years ago by exarkun

When #2078 was resolved, these docs probably should have been updated. There is no potential for a race anymore, and no warning will be emitted. Does that help at all? :)

comment:11 in reply to: ↑ 9 Changed 5 years ago by khorn

Replying to jesstess:

The reason I'd switched reactor.run to a plain <code> block was because the surrounding material keeps referencing reactor.fooMethod and the code uses reactor.run, and I thought a link to IReactorCore.run would be less clear.

How is this as a compromise:

reactor.<code base="twisted.internet.interfaces.IReactorCore" class="API">run</code>

so the namespace is consistent but only the "run" is a hyperlink?

IMO, I think this is fine. It may (will) create a spot we'll have to manually fix up after the Sphinx conversion, but there's already a jillion of those, so one more won't hurt a bit.

comment:12 in reply to: ↑ 3 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to jesstess

Replying to jesstess:

I also commented out XXX comments. I don't feel strongly about making them go away but I don't think they inspire faith in new users.

Can you open ticket(s) for those comments (if you think it's worth it)?

It's hard to verify that the new links are correct beyond eyeballing it, so a second pass by someone would be great.

I guess lore or the documentation builder doesn't check for broken (API) links. I checked most of them and they seem alright to me, +1 for merge, thanks.

comment:13 Changed 5 years ago by jesstess

(In [27987]) Remove reference to spawnProcess/reactor.run race condition.

refs #4194

comment:14 Changed 5 years ago by jesstess

(In [27988]) Remove hidden whitespace.

refs #4194

comment:15 Changed 5 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

(In [27989]) Merge bad-api-links-4194

Author: jesstess
Reviewers: khorn, glyph, exarkun, thijs
Fixes: #4194

Fix several broken API links in the core howto and comment out XXX todos.

comment:16 Changed 4 years ago by <automation>

  • Owner jesstess deleted
Note: See TracTickets for help on using tickets.