Skip to content
This repository was archived by the owner on May 4, 2021. It is now read-only.

Ticket27341#245

Merged
pastly merged 59 commits intotorproject:masterfrom
juga0:ticket27341
Sep 5, 2018
Merged

Ticket27341#245
pastly merged 59 commits intotorproject:masterfrom
juga0:ticket27341

Conversation

@juga0
Copy link
Copy Markdown
Contributor

@juga0 juga0 commented Aug 28, 2018

sbws install doc is confusing

juga0 added 30 commits August 28, 2018 14:31
Let's try to reuse terms and reuse definitions, terms are already
confusing enough.
and that can be also used in DEPLOY.rst
and that can be also used in DEPLOY.rst
and that can be also used in DEPLOY.rst
@juga0 juga0 added the docs Don't forget them! label Aug 28, 2018
Copy link
Copy Markdown
Member

@pastly pastly left a comment

Choose a reason for hiding this comment

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

See inline comments. They contain the only things that need to change for me to merge this PR.


Please don't make so many commits with the same message. All the "Add term used in the specification" commits could be the same commits. The "And remove it from README" commits should be combined with whatever they're referencing.

If you are doing this because I wrote "Strive to write many small commits each containing an atomic change instead of one large mega-commit." in CONTRIBUTING, this is swinging too far towards tiny commits. What that sentence is trying to avoid is mega commits where feature X is implemented, bugs Y and Z are fixed, and unrelated typos in documentation are fixed all in the same commit. It's trying to encourage feature X getting a commit, two commits for each of the bug fixes, and a commit for the typo fixes. Taking 10 commits to move documentation from one file to another and freshen it up is almost as bad as that above hypothetical mega-commit.

DEPLOY.rst Outdated
- Some sort of webserver installed and running that supports HEAD and GET
requests (apache and nginx fit this description)
- A Web server installed and running that supports HEAD and GET
requests (``apache`` and ``nginx`` fit this description)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It also needs to support Range headers, which both apache and nginx do. https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests

DEPLOY.rst Outdated
* ``~/.sbws/`` if you are running ``sbws`` manually
* ``/etc/sbws`` if you are running ``sbws`` from a system package as a
``systemd`` directory (not yet supported)
* any localion, an specify the path via the ``-c`` argument
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"localion" --> "location"

The only statement that is true here is that the file can be in any location if you use -c. If users want sbws to automatically find their config, they should not put it in ~/.sbws (how it used to be), they should place it at ~/.sbws.ini. Please remove the /etc/sbws line until the sbws debian package actually exists and locations are actually decided.

DEPLOY.rst Outdated
[destinations.bar]
url = https://barstoolsinc.com
url = https://example.com

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We no longer support automatically adding /sbws.bin as of ca37185#diff-8749426703045cfdcbbffb59a8a091b3

INSTALL.rst Outdated
The prefered method to install ``sbws`` is to install it from your system
distribution.
Currently there is not any system distribution package.
In the meanwhile, follow the following steps.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"meanwhile" --> "meantime"

INSTALL.rst Outdated

See the documentation section about configuration files for more information
about how to create a configuration file.
pip install .[dev] && pip install .[doc]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

".[doc]" --> ".[test]", see setup.py

INSTALL.rst Outdated
cd docs/ && make html

::
The generated HTML will be in `docs/build/`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.rst wants `` not ` for "code" formatting. This would work in markdown.

INSTALL.rst Outdated

Because we relay on a ``-dev`` version of stem, we need to fetch it from
git.torproject.org. Thus ``--process-dependency-links`` is necessary.
The generated man pages will be in `docs/man/`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.rst wants `` not ` for "code" formatting. This would work in markdown.

INSTALL.rst Outdated
~~~~~~~~

::
The generated diagrams will be in `docs/build/images/`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.rst wants `` not ` for "code" formatting. This would work in markdown.

@juga0
Copy link
Copy Markdown
Contributor Author

juga0 commented Aug 29, 2018

f you are doing this because I wrote "Strive to write many small commits each containing an atomic change instead of one large mega-commit." in CONTRIBUTING

Yes, and i started to liked :)

See #215 (comment). I've not contributed to CONTRIBUTING to focus on having a MVP.

I'm not usually happy with my commit messages, but it would be hard to accept any contribution from me if "nice" commit messages are required from me.

@pastly pastly merged commit 9239f69 into torproject:master Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

docs Don't forget them!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants