initialise base58 encode function #3
Conversation
README.md
Outdated
| @@ -1,2 +1,42 @@ | |||
| # elixir-base58-encode | |||
| https://hex.pm/packages/b58 | |||
|
|
|||
|
|
|||
| This module provide the `encode/1` function which takes a binary and returns | |||
There was a problem hiding this comment.
it's worth expanding on what a "binary" is.
A complete beginner could think that it is a series of 0's and 1's
equally it could be an executable binary
and the further down we present the usage example of Base58Encode.encode("foo")
"foo" is clearly a String not a "binary" (at least if you approach this from a beginner's perspective...)
README.md
Outdated
|
|
||
| and run `mix deps.get` | ||
|
|
||
| 2. Call the `encode` fundtion with a binary as parameter: |
README.md
Outdated
| 2. Call the `encode` fundtion with a binary as parameter: | ||
|
|
||
| ``` | ||
| > Base58Encode.encode("foo") |
There was a problem hiding this comment.
is "foo" a "binary" or a "String"?
see: https://elixir-lang.org/getting-started/binaries-strings-and-char-lists.html#binaries-and-bitstrings
README.md
Outdated
| if the parameter is not a binary the function will return `:error` | ||
|
|
||
| ``` | ||
| > Base58Encode.encode(42) |
There was a problem hiding this comment.
Surely 42 is an Integer? (it's "confusing" because 42 in binary is 0b101010...) 🤔
README.md
Outdated
| > "bQbp" | ||
| ``` | ||
|
|
||
| The alphabet used for base58 is: |
README.md
Outdated
| [How to encode a string to base58](https://github.com/dwyl/base58encode/issues/1) | ||
|
|
||
| Read the following for more information about binary in Elixir: | ||
| https://elixir-lang.org/getting-started/binaries-strings-and-char-lists.html |
There was a problem hiding this comment.
consider explaining further up why both "foo" and 42 are "binaries".
| iex> Base58Encode.encode("hello") | ||
| "Cn8eVZg" | ||
|
|
||
| iex> Base58Encode.encode(42) |
There was a problem hiding this comment.
README.md says that encode(42) is "bQbp" https://github.com/dwyl/base58encode/pull/3/files#r251960830
There was a problem hiding this comment.
The Readme was wrong! I've update the Readme to try to explain the difference between Elixir binary and binary numbers.
| 1 % 58 = 1 | ||
|
|
||
| So 65 is represented by 1 7 codes | ||
| ``` |
There was a problem hiding this comment.
This section isn't too clear to me. Why do we use modulo? Where does 1 % 58 come from?
There was a problem hiding this comment.
I didn't want had too much details on how to convert number from one base to another but I can try to describe the steps a bit more 👍
There was a problem hiding this comment.
Oh I liked this section actually.
Maybe add an example with rem/2 function (I know it does the same thing as modulo but maybe it will help to make it more clear to some people)? Just a thought
|
@nelsonic @Danwhy @RobStallion thanks for the review and all your comments, they are very useful! I think I've updated most of the part you have commented on and when you have some time it would be great to have your opinion again. |
| assert "Z" == Base58Encode.encode(" ") | ||
| end | ||
|
|
||
| property "Compare result with basefiftyeight package" do |
There was a problem hiding this comment.
Ahhhh that's really clever. I was wondering what a good way to add property tests to this would be. Good thinking @SimonLab 👍 😎
There was a problem hiding this comment.
A good "stopgap" until we can implement #4 and test against the "reference" C version
(and avoid comparing to the unmaintained one ... unless we want to run a perf benchmark?)
There was a problem hiding this comment.
see #4 for the next step on this (use the C library of base58 instead of the elixir package)
|
@SimonLab uh uh ... Travis is confused/unhappy. 😕 |
README.md
Outdated
| https://hex.pm/packages/b58 | ||
| [](https://hex.pm/packages/b58) | ||
| [](https://travis-ci.org/dwyl/base58encode) | ||
| [](https://coveralls.io/r/dwyl/base58Encode?branch=master) |
There was a problem hiding this comment.
I think that this link it meant to be to codecov
| ] | ||
| end | ||
|
|
||
| defp description() do |
Codecov Report
@@ Coverage Diff @@
## master #3 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 1
Lines ? 14
Branches ? 0
=======================================
Hits ? 14
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
| codes = get_codes(decimal, []) | ||
| leading_zeros(binary, "") <> codes_to_string(codes) | ||
| if decimal == 0 do | ||
| # see https://github.com/dwyl/base58encode/issues/5#issuecomment-459088540 |
|
@SimonLab thanks for fixing and adding docs. Hooray for property-based-testing. 🎉 |
There was a problem hiding this comment.
@SimonLab amaze! 😍
Docs, Tests and Code look great! 🎉
You have gone the extra mile on this and set a new standard for the rest of us to follow. 😮
Thank you! ✨
P.S. please integrate this into https://github.com/dwyl/cid tomorrow morning.
@Danwhy your feedback is still very much welcome/encouraged please comment on the PR and/or create issues! The only reason I merged the PR is because I feel it's "ready" and I don't want to be the "bottleneck" in Simon/Rob using this code in CID tomorrow. 👍
ref: #1
Encode string to base58
You should be able to call the function similar to