Radical makeover of docsite.

Review Request #3767 — Created April 26, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, lahosken, patricklaw, stuhood, zundel
+ Changes to content: Removed old/outdated/unhelpful content,
  added new content, rephrased various paragraphs to be
  more succinct and/or pertinent to 2016 Pants.

+ Changes to styling, using a customized version of Bootstrap.

I don't think looking at diffs will be particularly instructuve,
since I've made big changes everywhere. It might be better
to look at the content directly:

http://pantsbuild.github.io/staging/_new_design_/

CI pending: https://travis-ci.org/pantsbuild/pants/builds/125961756

  • 0
  • 0
  • 11
  • 3
  • 14
Description From Last Updated
  1. Is it possible split this into a code doc and style review? Not sure how tightly coupled the style, doc and code changes are. It will probably make the RB easier to digest.

    1. The style and content changes are somewhat coupled, not horribly tightly. The code changes are pretty minimal, and it's obvious which files they're in. But apart from those, I don't think looking at the RB diffs will be instructive in this specific change.

      In other words, I don't think this needs a line-by-line diff review. I'd rather get comments on the design and on egregious content problems.

    2. Sounds good to me, I'll keep my contents mostly to the staging version you have posted then. First thanks for taking this on, overall it looks great.

      • It looks like we lost the build status badge, is that intentional?
      • The image of a pants run is really small and hard to read on my computer. The image doesn't seem to be clickable to see the actual image.
      • The header looks strange on high res displays because Pants and the search don't line up with either the column of text or the left margin... it just kind of hangs out there.
      • Could we add the slack self invite link to the community page?
  2. 
      
  1. Thanks for working on this Benjy... big improvement.

    Colors wise: every time you git the spacebar, this generator will generate a new palette to go with our blue: https://coolors.co/app/5fb1ef-ffffff-ffe8d1-568ea3-826251

    1. Maybe I'm not using it right, but this is just generating sets of compatible colors, right? There doesn't appear to be any particular suitability for our blue (#55acee).

    2. The first column on that link was locked to what I thought was our blue, but which was actually a few shades off. https://coolors.co/app/55acee-e8eef2-d6c9c9-c7d3dd-37393a ... if you click the lock icon on a column it will lock in that color, but apparently that is lost in the pasted link.

    3. Ah, got it. I used this to generate a darker color for links.

  2. src/docs/community.md (Diff revision 1)
     
     

    This should likely lift directly from http://pantsbuild.github.io/howto_contribute.html#join-the-conversation

    1. Maybe I'm not using it right, but this is just generating sets of compatible colors, right? There doesn't appear to be any particular suitability for our blue (#55acee).

    2. Er, right comment, wrong place...

    3. I copied over some more of that text, but this is not the same thing as wanting to hack on Pants, so some of that text isn't relevant here.

  3. src/docs/index.md (Diff revision 1)
     
     

    We support javascript as well via the node.js plugin.

  4. Unsorted comments:

    • The page title for the main landing page (index.html) is "index".
    • The RELEASE_HISTORY is still capitalized.
  1. You are right, looking at the diff isn't too helpful - there have been a lot of changes. I'll start reviewing the site.

  2. build-support/bin/publish_docs.sh (Diff revision 1)
     
     

    do you want to leave this on?

    1. It's useful for seeing which pants commands the script runs, so yes unless you object.

  3. nit: trailing space here and in other places. I won't mention it again.

    1. Harrumph. I have intellij set to strip trailing whitespace on all lines in all files it saves (not just lines I edited), so I'm not sure why this happens. Will sed this...

  4. src/python/pants/docs/docsite.css (Diff revision 1)
     
     

    I think this would look better with some margin on the bottom, like:

    margin: 1em 0 .3em -0.01em;

    1. I am, to put it mildly, not a web designer...

  5. 
      
  1. Thank you for working on this! So far I like it a log. Unfortunately, I only got through three pages tonight

    • Installing Pants
    • Setting Up Pants
    • Tutorial
  2. I don't know if its always been this way, but this doesn't render well.

    1. I didn't touch this, but I've fixed it up now (I think).

  3. src/docs/first_tutorial.md (Diff revision 1)
     
     

    These are rendering under the sidebar on the right hand side of http://pantsbuild.github.io/staging/new_design/first_tutorial.html

    looks funny

    1. Yeah, they always did (if you check the live site you'll see it), it just wasn't as noticeable under the old color scheme.

      My CSS-fu is nowhere near good enough to fix this, but I will play around and see what I can do.

      Update: My CSS-fu is not so bad after all, was able to fix this.

  4. src/docs/install.md (Diff revision 1)
     
     

    make typewriter font? pants.ini ?

  5. src/docs/setup_repo.md (Diff revision 1)
     
     

    this is not displaying as a single column list

  6. src/docs/setup_repo.md (Diff revision 1)
     
     

    This looks really weird rendered as a bullet point. Should it be another heading? Is it just trying to provide a link?

    1. Yes. I thought the single bullet looked good for emphasis, but I've inlined the link into the preceding text.

  7. src/docs/setup_repo.md (Diff revision 1)
     
     

    This doesn't render with what looks like padding between the bullet list above and the start of the next paragraph (maybe it could be solved with extra margin-bottom: in CSS?

    1. Looks like we set the padding and margins for ul, ol and dl to all zeros in the css. I added some bottom margin.

  8. src/docs/setup_repo.md (Diff revision 1)
     
     

    href for Varnish to https://www.varnish-cache.org/

    1. Fixed. Also added a link for nginx.

  9. src/docs/setup_repo.md (Diff revision 1)
     
     

    This doesn't render right. I think you want :::python ?

    1. I guess I wanted :::ini.

  10. src/python/pants/docs/docsite.css (Diff revision 1)
     
     

    I think it looks better with more padding: e.g.

    padding: 5px 10px 5px 10px;

  11. 
      
  1. Yay for the new bootstrap look.
    
    pex_design.md gets deleted. Yay, I agree it doesn't belong w/the Pants docs. Do we give the Pex folks a heads-up so they can salvage parts they want? (Maybe we already did a long time ago? I'm hazy. If it happened, it was a long time ago.)
    1. I don't know. In fact, I don't even know if pex_design.md is up-to-date. I'll give pex ppl a holler though.

    2. The pex housed docs are more up to date and a more appropriate home:
        https://github.com/pantsbuild/pex/tree/master/docs
        https://pex.readthedocs.io/en/stable/
  2. 
      
  1. I don't want to hold up progress, but there is a lot of documentation that was deleted or moved or something and I don't feel good signing off on it. I'll try to review more tonight.

    1. I definitely outright deleted some stuff. Either because it was out of date, or irrelevant to docsite visitors.

    2. Can you push a branch and ? I can't apply the diff that I downloaded from rbcommons. Some of the stuff I saw in red I thought was important but maybe you moved it elsewhere.

  2. 
      
  1. Crap, something is leaking in our apartment and I need to go attend to it.

  2. src/docs/first_concepts.md (Diff revision 4)
     
     

    This summary needs to summarize the content below:

    To use Pants effectively, it helps to understand a few concepts:

    • goals tell pants what actions to take
    • targets describe what files to take those actions upon
  3. src/docs/why_use_pants.md (Diff revision 4)
     
     

    The first few paragraphs of this page are fragmented.

    ideas here: https://docs.google.com/document/d/1vMQsrOJHrorUAqf1qV5OdbWXppzSWRRUDiqQIzQcLaw/edit#bookmark=id.ukhsgnco4glu

  4. 
      
  1. 
      
  2. src/docs/first_concepts.md (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This section is redundant with the new top level page "why_use_pants.md"

  3. 
      
  1. 
      
  2. src/docs/build_files.md (Diff revision 4)
     
     

    This is now the only mention I could find of the 1:1:1 rule in the documentation and it only explains what two of the three '1's are.

    1. The text says "one target per directory, representing a single package", so I think that does reflect the three 1s: 1 target, 1 directory, 1 package.

      And since it's just a recommendation, not really a rule, I think this being the only place we mention it is appropriate. Where would you add it?

  3. src/docs/build_files.md (Diff revision 4)
     
     

    this heading should be 'java_library' to be consistent with the other list headings

    **java_library**
    Each target will have a different _target type_, which is `java_library` in this case.  This tells pants tasks what can be done with the target.
    
  4. src/docs/build_files.md (Diff revision 4)
     
     

    This has been there all along but this is confusing when you read it together with the next suggestion

    Use list:

    Use list ::

    So is the colon a part of the first command?

    We could fix this by rephrasing

    *What targets does a BUILD file define?* Use the `list` goal:
    
    ...
    
    
    *Are any BUILD files broken?*
    List **every** target to see if there are any errors:
    Use the  recursive wildcard `::` with the list goal: 
    
  5. src/docs/build_files.md (Diff revision 4)
     
     

    I don't think this is the right place to talk about buildgen. Its not a feature that's available for general use and there is no such thing in existence for the most popular languages we use at square (python & java). Maybe a "future releases" section where we could mention this and the engine?

    1. Yeah, I wanted to reassure readers that we know that this verbosity and boilerplate needs trimming, but you're right that this isn't the place, and we probably shouldn't mention it at all until there's something more solid to mention.

  6. src/docs/build_files.md (Diff revision 4)
     
     

    the BUILD.* feature is useful. This is the only reference to it in the docs. Could you revive it?

    1. Not sure what you mean? It is mentioned under "BUILD.* files" on line 96.

  7. src/docs/target_addresses.md (Diff revision 4)
     
     

    I think the concept of 'address' deserves a mention in the 'first_concepts' page.

    1. I didn't touch that file at all (one of the few such files), so I'd rather not drag it into this change now. Let's follow up with improvements to that page in a separate change?

  8. src/docs/target_addresses.md (Diff revision 4)
     
     

    Maybe this caveat should preceed the two definitions instead of after. Further down we talk about globbing which is also not allowed in BUILD files. I think they should all go under a common heading.

    1. Again, this was the structure in the original file, so let's fix up this page in a separate change?

  9. 
      
  1. "PEX-based Installation" under http://pantsbuild.github.io/staging/new_design/install.html may confuse new users. May be good to include a link to what pex is.

  2. 
      
  1. 
      
    1. Can we break out design feedback from text-feedback? It would be good to land the design aspect and then review the posted site afterward more iteratively.

    2. ...in essence, I'm saying: let's ship this and then iterate on polishing it and restoring removed sections, what-have-you over the weekend.

    3. Yes, I think that's a good idea. Once I address Eric's comments I think this will be good enough to ship and then iterate on in follow-up commits.

  2. examples/src/java/org/pantsbuild/example/3rdparty_jvm.md (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Rewrite for this section. The workaround described hasn't been necessary for a long time.

    It is possible for your code to exist in the repo but also be published to an external repository. If you happen to pull in any third party artifacts, they may express a dependency on the published version of the artifiact. This means that you'll have both the version in the repo compiled from source on the classpath and an older version that was previously published. Potentially, both libraries could end up on the classpath. In this case, you want to be sure that when pants always prefers the version built from source.

    Fortunately, the remedy for this is simple. If you add a provides parameter that matches the one used to publish the artifact, pants will always prefer the local target definition to the previously published jar.

    jar_library(name='api',
          sources = rglobs['*.java'],
          artifact(org='org.archie',
                   name='api',
                   repo=myrepo,)
    )
    
    1. Can you do this rewrite after I push this? I'm not sure I know exactly which part to excise and replace with this text.

  3. FYI, I've asked @gmalmquist to draft a section on managed dependencies for this page. This seems like the right place for it to go.

  4. src/docs/invoking.md (Diff revision 4)
     
     

    Many goals naturally have only one task, in which case you omit the repetition by using the shorthand flags:

        :::bash
        ./pants list --list-sep='|' examples/src/python/example:
    
    Can also be written as:
    
        :::bash
        $ ./pants list --sep='|' examples/src/python/example:
    
  5. src/docs/invoking.md (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Why did you decide to remove all of this? I know that it is enough to tell a python programmer that 'this file follows the sytle of a python ini file' but I don't think that's the audience. (The ratio of non-python programmers to python programmers in our shop is about 100:1)

    1. This moved to the new page on options. The "invoking" page seemed like the wrong place for this.

    2. None of this deleted text made it to the options page. I'm going to propose reinstating it in a followon PR

    3. The text didn't make it as-is, no. It was rephrased. But as far as I can tell all the content in this deleted block is covered in the new options page.

  6. 
      
  1. Thanks for getting this ball rolling again Benjy.
  2. 
      
  1. Ship It!
  2. 
      
  1. I will probably continue referencing this review because I haven't reviewed all the deleted content, but I think at this point it would be better to land this and iterate, that way I can post my own changes.

  2. Missing period

  3. src/docs/ide_support.md (Diff revision 5)
     
     

    missing period at the end of this sentence.

  4. 
      
  1. Submitted as acb34dadfa7e761d38721e6c26ee75938e3faf75! Thanks for all the helpful comments, especially to Eric, who slogged carefully through the whole thing. I'm pretty weary of the docsite now, but feel free to create further changes, add, change, move, remove content, etc.

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

acb34dadfa7e761d38721e6c26ee75938e3faf75

Loading...