'goal builddict' : generate .rst and .html, not just .rst

Review Request #1079 — Created Sept. 26, 2014 and submitted

lahosken
pants
529
d791c0c...
pants-reviews
patricklaw

before: running 'goal builddict' generates a couple of .rst files which we then give to Sphinx

after: running 'goal builddict' generates those .rst files but also a couple of .html files which we then give to sitegen.

If/when sitegen's in good shape, it'd be good to get rid of the .rst-generating part. (And if you say this code should change to separate out the rst-codepath from the html-codepath, I won't argue; I dithered over that. As it is now, the code tends to compute rst, compute html, use rst, use html and it's all lumped together. It kinda makes sense since some of that html is converted from rst.)

Most of this change is in src/python/pants/backend/core/tasks/builddictionary.py , plus new html templates.

There are also changes to some docstrings:

I replaced some sphinx-rst-isms with docutil-rst-isms (which also work with Sphinx).

And I replaced some lost docstrings which I removed with the "inherit docstrings" change because I "knew" they'd be inherited from a superclass.

And added some docstrings which have been missing for a while which I noticed because increased scrutiny grrrrr.

Maybe I should kick the docstring changes, especially the ones not directly-related to rst-fixing, out of this change, into their own change. I'm hoping they don't increase the, uhm, cognitive load of this change since they're "just docstrings", but I dunno.

https://travis-ci.org/pantsbuild/pants/builds/36888156

Running sphinx on the generated .rst files now generates warnings :-(
I poked around 'em for a while, but eventually gave up.

  • 0
  • 0
  • 8
  • 1
  • 9
Description From Last Updated
LA
PA
  1. 
      
  2. 3rdparty/python/requirements.txt (Diff revision 1)
     
     

    When adding or updating third party dependencies, prefer ranges. See e.g. coverage right above.

  3. Prefer a more descriptive variable name

  4. Pretty much 100% of the time, always have a newline after a colon, even if it's a one-liner. In extremely rare examples (just subclassing an Exception type with no overrides) it's okay to inline the pass, but even then it's questionable if the exception is worth it.

  5. Yikes, this seems super fragile. If it's really not worth it to do this properly, please at least add a comment indicating why.

    1. Maybe fixed; maybe not. Changed the docstring that tries to say what "span" means.

  6. I'd do len(methods or []) > 0

  7. More descriptive variable name. And instead of the or, you can just default to ''. Since strings are immutable in python, this isn't risky like it would be with a list.

    1. instead of the or, you can just default to ''

      Alas, sometimes, we pass in None, though it's in disguise as foo.doc.

  8. if any(r.match(line) for r in [param_re, type_re]):

    1. oh, nice. this let me clean up the following logic, too

  9. 
      
LA
LA
PA
  1. 
      
  2. Is there any reason not to let it do what it likes, but squelch the visual issues in CSS? This is still quite fragile (string manipulation to parse HTML), and it's not obvious to me what the complexity of this helper buys us.

    1. Here's a version that squelches the visual issues in CSS.

      The worries are tiny:

      if docutils starts generating <p style="something:relevant;"> for some interestingly-formatted rst, we prrrobably don't want to squelch that p. We prrrobably only would want to squelch plain vanilla <p>. No reason to think docutils will do that anytime soon. And if it starts, no reason to think the consequences will be consequential.

      'tidy' whines about a p in a span.

    2. lgtm

  3. I think it's actually much easier to read here if you nest the second if within the first and inline the recording_state expression. That way there's no "state" at all in this loop.

    1. changed. I wasn't 100% sure what you wanted, so maybe done, maybe not.

    2. What I'm saying is, what is the point of recording_state as a variable at all?

      for line in docstring.splitlines():
        if line and not line[0].isspace():
          if not any(r.match(line) for r in [param_re, type_re]):
            body += line + '\n'
      
    3. ah wait, I see what's going on now. Nevermind. This new version is fine.

  4. 
      
LA
LA
LA
  1. rattle I see some happy comments. I don't see a "Ship It". Mmmaybe that means you're happy and just didn't press the "Ship It" button. If you're still thinking it over, that's cool, too.

  2. 
      
PA
  1. Sorry, thought I'd hit it earlier!

  2. 
      
LA
LA
Review request changed

Status: Closed (submitted)

Loading...