steemit / simple_steem_client

A minimalist Steem RPC client in Python
2 stars 5 forks source link

Pure Python serializer #6

Closed goldibex closed 6 years ago

goldibex commented 6 years ago

Closes #3.

Serializer manages its own data buffer (as a bytearray) and provides facilities for efficiently retrieving the serialized data. It has no dependencies.

I've also added a Dockerfile, Pipfile, and Pipfile.lock.

@theoreticalbts, if at all possible I'd like to get some real-live test vectors (i.e. binary data of actual transactions, etc.) to include in the test units.

theoreticalbts commented 6 years ago

In my experience, pipenv tends to break things and/or make the installation process non-obvious. I want to be sure that either we do not add pipenv, or if we do, take steps to ensure nothing is broken and the installation process is specifically documented for the benefit of people (like myself) who have never used it (or in my case, never used it successfully).

I had a bad experience with pipenv in steem-python. One of the reasons I created simple_steem_client was to have a library that does not have similar installation headaches. So I am very leery about adding pipenv to simple_steem_client, and I would like to proceed without it if possible.

Why do we need pipenv as a dependency? What benefits does it provide?

Given this new packaging, what is the proper way to do common tasks such as:

I tried to do what seemed obvious to me with steem-python, and the results were bad: https://github.com/steemit/steem-python/issues/96

FWIW I've done plenty in Python, but I'd never heard of pipenv until I started digging into steem-python and ultimately filed the above issue. Usually I do something like

virtualenv -p $(which python3) ~/ve/myproject
. ~/ve/myproject/bin/activate
pip install some_other_project

and then use requirements.txt to concisely express the list of dependencies or lock specific versions. It seems that maybe pipenv replaces some parts of this "standard" workflow but it is not obvious, my sole experience being the above-documented, very frustrating failed attempt to get steem-python working.

My default position is this:

If pipenv has some benefits, or if it's some kind of de facto standard in the Python ecosystem or in Steemit, I might budge from this position and allow it to be added.

However, if we do move forward with adding pipenv, I would like to have a guide which specifies exactly a sequence of commands that can be used to accomplish tasks (a)-(e), so if I (or anyone) has installation issues like the above mentioned ticket, it will be possible to figure out from the documentation whether the problem is the user's incorrect or incomplete understanding of the packaging system, or if it is some actual bug in the packaging data.

goldibex commented 6 years ago

I'll just remove pipenv from this PR, though I should point out that it is now the officially recommended Python packaging tool. Can you review the serializer proper?

theoreticalbts commented 6 years ago

The serializer looks OK, there are no obvious bugs.

As to your question about serialized transactions:

goldibex commented 6 years ago

@theoreticalbts Pipfile now removed, any other feedback or LGTY?

theoreticalbts commented 6 years ago

LGTM, go ahead and hit the Merge button