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

Add user, installation and testing guide #112

Closed
wants to merge 13 commits into from
Closed

Conversation

ganeshrn
Copy link
Member

SUMMARY

Add user, installation and testing guide

ISSUE TYPE
  • Docs Pull Request
ADDITIONAL INFORMATION


Copy file from remote host
===========================
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

There should usually be some whitespace before code-blocks

Suggested change
.. code-block:: python
.. code-block:: python

Comment on lines 1 to 3
##########
User Guide
##########
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to use the same style for titles across the project

Suggested change
##########
User Guide
##########
==========
User Guide
==========

Comment on lines 16 to 22
Running tests
==============

.. code-block:: shell-session
$ tox -e test-binary-dists
Copy link
Member

Choose a reason for hiding this comment

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

This should mention that it expects the test targets to be pre-built in ./dist/

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Also, the URL nesting is too deep. This project is not as complex as Ansible Engine and doesn't need that. Besides, it's weird to have to navigate this deep in almost empty docs. Let's have just flat files without dirs or nested indexes and, if needed in the future, we'd move things around.

@@ -0,0 +1,25 @@
.. _testing_guide:
.. _unit_testing_guide:
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be "unit-testing" it's just testing.


ansible-pylibssh also uses the following Python modules that need to be installed [1]_:

.. code-block:: bash
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. code-block:: bash
.. code-block:: shell-session

Comment on lines 53 to 71
ansible-pylibssh can be installed from source::

$ git clone https://github.com/ansible/pylibssh.git
$ cd pylibssh
$ pip install tox
$ tox -e build-dists

`manylinux`-compatible wheels::

$ git clone https://github.com/ansible/pylibssh.git
$ cd pylibssh
$ pip install tox
$ tox -e build-dists-manylinux # with Docker

# or with Podman
$ DOCKER_EXECUTABLE=podman tox -e build-dists-manylinux

# to enable shell script debug mode use
$ tox -e build-dists-manylinux -- -e DEBUG=1
Copy link
Member

Choose a reason for hiding this comment

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

This should probably just include a part of README instead of a copy


Prerequisites
--------------
You need Python 2.7 or 3.5+
Copy link
Member

Choose a reason for hiding this comment

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

There should be some whitespace after RST titles

Suggested change
You need Python 2.7 or 3.5+
You need Python 2.7 or 3.5+

README.rst Outdated
@@ -63,35 +63,6 @@ pylibssh requires libssh to be installed in particular:
<https://www.libssh.org/get-it/>`__.


Building the module
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing, use comment markers like .. DO-NOT-REMOVE-SOME-UNIQUE-NAME in order to single-source parts of README into the docs.

.. code-block:: python

chan = ssh.new_channel()
print("stdout:\n%s\n stderr:\n%s\n returncode:\n%s\n" % (resp.stdout, resp.stderr, resp.returncode))
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what resp is here.

Comment on lines 15 to 19
.. code-block:: python

from pylibsshext._libssh_version import LIBSSH_VERSION

print(LIBSSH_VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of putting Python sources in RST, let's put them in Python files and just include their contents (full or partial). It's easier to maintain files with complete sources that are just included in guides in parts. And it's possible to lint/check those files too, unlike code blocks in sphinx.

Copy link
Member

Choose a reason for hiding this comment

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

(you may delegate this to me if you want as I have a lot of experience using the inclusions)

.. _intro_getting_started:

***************
Getting Started
Copy link
Member

Choose a reason for hiding this comment

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

I think this should specify that this is low-level/paramiko-like API

Getting Started
***************

Now that you have read the :ref:`installation guide<installation_guide>` and installed ansible-pylibssh on a your system.
Copy link
Member

Choose a reason for hiding this comment

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

It's better to maintain spaces for readability + typo

Suggested change
Now that you have read the :ref:`installation guide<installation_guide>` and installed ansible-pylibssh on a your system.
Now that you have read the :ref:`installation guide <installation_guide>` and installed |project| on your system.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, this would work too

Suggested change
Now that you have read the :ref:`installation guide<installation_guide>` and installed ansible-pylibssh on a your system.
Now that you have read the :ref:`installation_guide` and installed |project| on your system.

Comment on lines 21 to 22
Creating a SSH session
======================
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Creating a SSH session
======================
Creating an SSH session
=======================

@webknjaz webknjaz added the documentation Improvements or additions to documentation label Jul 27, 2020
@webknjaz webknjaz self-assigned this Jul 28, 2020
@webknjaz webknjaz marked this pull request as draft July 28, 2020 15:52
@webknjaz webknjaz force-pushed the docs branch 2 times, most recently from 0a6e9e4 to a5b711b Compare August 3, 2020 16:04
Comment on lines +35 to +33
|project| can be installed with ``pip``, the Python package manager. If ``pip`` isn't already available on your system of Python, run the following commands to install it::

$ curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py
$ python get-pip.py --user
Copy link
Member

Choose a reason for hiding this comment

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

This is a very bad idea to advertise. Link to the PyPA packaging docs instead.

TODO: get rid of it.


.. note::

Older versions of ``pip`` default to http://pypi.python.org/simple, which no longer works.
Copy link
Member

Choose a reason for hiding this comment

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

This is not really true, there's a redirect.

TODO: Stop vendoring a copy of the PyPA guides, link there instead.

@webknjaz webknjaz force-pushed the docs branch 3 times, most recently from 2962586 to 8c38119 Compare August 7, 2020 18:25
@webknjaz
Copy link
Member

webknjaz commented Sep 1, 2020

I've split this into #141, #142 and #145.

@webknjaz webknjaz closed this Sep 1, 2020
@webknjaz webknjaz linked an issue Sep 1, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TODO] Document how development and testing works
2 participants