Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prerequisites #214

Closed
wants to merge 7 commits into from
Closed

Prerequisites #214

wants to merge 7 commits into from

Conversation

JoshuaAcosta
Copy link

No description provided.

@JoshuaAcosta JoshuaAcosta deleted the prerequisites branch January 24, 2016 13:29
@alexwlchan alexwlchan mentioned this pull request Jan 24, 2016
@JoshuaAcosta JoshuaAcosta restored the prerequisites branch January 24, 2016 19:20
@JoshuaAcosta
Copy link
Author

I've created a rough draft of the prerequisites section. Please feel free to provide feedback on style, grammar, wording or anything else you see fit. Thanks.

@JoshuaAcosta JoshuaAcosta reopened this Jan 24, 2016
Prerequisites for Creating Packages
==========================

In order to create or install packages, you must have pip, setuptools and wheel installed. The section below describe the steps for installation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/s/describe/describes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to reframe this section a little bit. Rather than saying that we need those tools to "create or install packages", I'd like to be more clear that these are the tools required to complete this particular tutorial. (These aren't the only tools out there, but they are the only tools we're going to require to complete the tutorial.)

@ddbeck
Copy link
Contributor

ddbeck commented Jan 26, 2016

@JoshuaAcosta Thanks for getting started on this! I really appreciate you taking the lead on this. I agree with @alexwlchan's comments and a I put a couple of my own. Overall, I think this is a great start and seeing your draft has illuminated some of the challenges of this section for me.

One way we might improve this section would be to back away from assumptions about the reader's system (e.g., that they'll necessarily have x and y but not z installed) and instead check each tool in turn and provide a path forward given the results (e.g., run pip -V and say, here's how to get pip if you need it and here's how to upgrade if you don't). Would you be interested in revising it?

After that, there's some nitpicky formatting issues to deal with, but I think we can work out the overall content a bit before dealing with that. Thanks again for getting started on this!

@JoshuaAcosta
Copy link
Author

Thank you very much for the feedback. I'll push the revision with a day or two.

@JoshuaAcosta
Copy link
Author

@ddbeck Please feel free to provide some feedback on the revision. I changed the overall content a bit to reflect the flow for each tool you mentioned.

*To install pip, setuptools and wheel, you must have a version of Python installed. You can verify if
there is a version of Python in your system by entering the following into your terminal or command prompt:

python -V
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing, but I prefer using the full version of the flag:

python --version

which makes it completely obvious what this should return.

It might also be helpful to have some explanation of what the output should be – as below, something like “if a version number is returned, then Python is installed”.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding this. Using the non-abbreviated flag and describing the expected output is a good idea.

@ddbeck
Copy link
Contributor

ddbeck commented Feb 7, 2016

@JoshuaAcosta I like the changes you've made with this revision. Thank you! Here's how I think we should proceed:

  • Alex is right about the issues he mentioned in his comment. To be fair, I should've been clearer about that in my original suggestion to you. 😊 If you're willing to update your PR to address that, that would be great.
  • As a matter of content, I think this section is looking pretty good, the aforementioned issue aside. There's some things we can do to expand this later (like recapping and previewing at the end of the section), but I think those changes would depend on having the other sections filled out a bit more anyway.
  • From here, I think the changes I'd like to see relate to dressing up the formatting. If you have or want to get some more experience with reStructuredText, I can point out some changes to make. Alternatively, I can merge your work and make formatting and style changes myself.

If you'd let me know what you have the interest and time to do, we can go from there. Thanks again for your work on this section!

@JoshuaAcosta
Copy link
Author

@ddbeck Thanks for the feedback! I'd like a little more experience with reStructuredText so any pointers or helpful reference material would be appreciated.

@ddbeck
Copy link
Contributor

ddbeck commented Feb 24, 2016

@JoshuaAcosta Great! Here's my notes on formatting for you:

  • Before making your other changes, I'd suggest taking a look at Sphinx's reStructuredText Primer, which outlines how to format text for reStructuredText/Sphinx. It's similar to the Markdown markup used here on GitHub, but it's got enough differences that you'd probably benefit from skimming that page.
  • The first major formatting change would be to promote the pip, setuptools, and wheel parts into sections with section headings. This makes them show up in the table of contents and breaks up the page a bit to help readability.
  • The next major formatting change would be to format the sample commands as code samples (enclosing them in double backquotes like this). Most of them are enclosed in double or single quotes in the current PR.
  • Lastly, the link to get-pip.py isn't formatted for rST and ought to be fixed as well.

If you have Sphinx installed, you can run make html from the root of the repository, then open build/html/prerequisites.html in a browser to check out your handiwork.

Let me know if you have any questions. I'm happy to help you out on this. Thanks again!

@JoshuaAcosta
Copy link
Author

@ddbeck Made the formatting corrections you suggested. Struggled to get the hyperlink formatting right, is it correct? Let me know if anything else could use some work. Thank you for the rST guidance!

@alexwlchan thanks for your help!


====
pip
====
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a sub-section, we need to use - instead of =:

pip
---

@ddbeck
Copy link
Contributor

ddbeck commented Mar 11, 2016

@JoshuaAcosta Nice start on the formatting. The link works now. But one of the big areas that needs improvement though is the formatting of the commands. As before, it would be good to enclose them in double backticks so they stand out from the other text. Is that something you'd be comfortable taking on?

If so, I'd suggest getting local builds up and running, in the root of the project:

$ pip install Sphinx   # if you haven't installed Sphinx yet.
$ make html            # or `make.bat html` on Windows

And then open build/html/prerequisites.html to see your handiwork. When you make changes to your file, rerun the make html line to rebuild the output. That way you can test your formatting changes before updating your pull request.

Let me know what you plan to do. Thanks!

@JoshuaAcosta
Copy link
Author

@ddbeck I've updated the file with the feedback provided. Please let me know if there are any other changes to be made. Thank you for your patience!

@berkerpeksag thanks for the feedback!


To install pip, setuptools and wheel, you must have a version of Python
installed. You can verify if there is a version of Python in your
system by entering the following into your terminal or command prompt.::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've used :: to start code blocks, which is fine, but this is a bit of a shortcut that assumes you want to end the paragraph with a colon as well as start the following code block. So what we've got now is each of these, as built by Sphinx, ending with something along the lines of or command prompt.:. So you'll need to either need to drop the ending periods, or replace the double-colon code-blocks with a more verbose version like this:

…or command prompt.

.. code-block:: shell

   python --version

Personally, I always go with the verbose version, since it's harder to surprise myself with punctuation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the periods would be a good start, since almost all the last sentences that :: affixes are instructions to do something, not proper sentences.

@ddbeck
Copy link
Contributor

ddbeck commented Mar 29, 2016

@JoshuaAcosta I like how this is shaping up! I made a comment on a formatting issue—that was the only thing that jumped out at me. Otherwise, it looks like we can wrap this up soon!

@JoshuaAcosta
Copy link
Author

@ddbeck I've updated the doc with the verbose version you suggested. Let me know what you think.

@ddbeck
Copy link
Contributor

ddbeck commented Apr 5, 2016

@JoshuaAcosta thanks for doing that! I think we're in pretty good shape and we're ready to wrap this up. Here's how I plan to proceed:

First, I'm going to merge your PR. Unfortunately, I need to target a different branch than the one you selected when you initiated the PR, so I'm going to merge this manually (on to develop) and then close this PR. You'll still be credited in the commit log, but it will appear as if this PR was closed and not merged, strictly speaking.

Second—and this is as much for the other folks following on this issue as it is for you—I don't expect this will be the final iteration of this section. I'm going to turn my attention to the other sections and get some other things written. After that, I'll likely return to this section with some style and consistency changes. So if you want to follow along or contribute on that front, I think watching #194 is the way to go.

Thanks for the time and effort you put into this! I really appreciate it.

@Ivoz
Copy link
Contributor

Ivoz commented Apr 9, 2016

@ddbeck maybe you want a CONTRIBUTING.md on the project / a note in the readme to advise ppl to contribute to develop?

Or even set develop to be the default branch in Github.

ddbeck pushed a commit that referenced this pull request May 3, 2016
This is for issue #196 and PR #214.
@ddbeck ddbeck closed this May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants