BlockchainCommons / seedtool-cli

Cryptographic Seed Tool for the command line
Other
25 stars 16 forks source link

Add Dockerfile. Fixes #44. #54

Closed secinthenet closed 3 years ago

secinthenet commented 3 years ago

Continuation of a previous PR #47

gorazdko commented 3 years ago

Thanks for the PR. Looks good to me, will test it today.

I'm not sure why linter suddenly decided to throw an error here but doesn't have any issues with it in master:

19/20 files checked 92% done
Checking utils.cpp ...
utils.cpp:135:21: style: Condition 'c==separator' is always true [knownConditionTrueFalse]
        } else if(c == separator && buf.length() > 0) {
                    ^
utils.cpp:133:8: note: Assuming condition 'c!=separator' is false
  if(c != separator) {
       ^
utils.cpp:135:21: note: Condition 'c==separator' is always true
        } else if(c == separator && buf.length() > 0) {
                    ^
20/20 files checked 100% done

And on linux:

12/20 files checked 60% done
Checking format-sskr.cpp ...
[format-sskr.cpp:45]: (error) Memory leak: bytes_buf
13/20 files checked 65% done

which is a false positive imo.

Anyways, the linter problems are on our side not caused by your PRs.

gorazdko commented 3 years ago

I have build and tested. Works fine.

shunit2 is introduced as a submodule now which i'm fine with, it's just a dev dependency

@ChristopherA I think we can merge this in despite the linter failing. I'll reapply the linter fix (#57) subsequently. Also let me know if we want to document shunit2 as a dev dependency (reason is #55) to our README, and i'll add that too

ChristopherA commented 3 years ago

Merging PR despite lint errors — @gorazdko will fix in subsequent PR so @secinthenet doesnt have to rebase.

@secinthenet: in the future could you enable “allow edits by maintainers” in your PRs? It makes it easier for us to fix these type of things.

gorazdko commented 3 years ago

CI was fixed automatically. So no issues now.

I had asked Wolf to manually reset the CI but said that the error persisted, so i wasn't sure what was wrong.