search

goat-systems / go-tezos

Go Tezos Is a Go library that exposes and builds upon the Tezos RPC.
MIT License
71 stars 45 forks source link

constructor makes no sense when used as library #90

Closed tulpenhaendler closed 5 years ago

tulpenhaendler commented 5 years ago

When i try to use go-tezos as a library in another go programm, i get:

[] Error loading .env file: open .env: no such file or directory

when constructing with: 

ngt,err := gotezos.NewGoTezos("http://" + e.Tezos.Container.Ips[0]+":8732",)

this is caused by:

https://github.com/DefinitelyNotAGoat/go-tezos/pull/84/commits/4f80fbc8e884808a890879798da952f66c71a299

this makes no sense to me, why is the URL parameter ...string when multiple urls are not even used,

if i construct with 

ngt,err := gotezos.NewGoTezos("http://" + e.Tezos.Container.Ips[0]+":8732","nowthisworks")

if works obviously, because URL[0] now returns the entire address, whereas before it would evaluate to "h" which does not have length > 1 and force it to laod a .env file.

tulpenhaendler commented 5 years ago

made a mistake describing this in the last sentence: len(URL) does not have length > 1 with the first constructor -> .env load

RomarQ commented 5 years ago

That problem was caused by the last refactored code pushed by @DefinitelyNotAGoat , use version 1.0.8 while @DefinitelyNotAGoat doesn't update all the changes he has made.

RomarQ commented 5 years ago

Also, the next proposal will require some major changes, and probably will be better to delay version 2.0.0 for when it comes out.

tulpenhaendler commented 5 years ago

that is months away and this is an easy fix :(

RomarQ commented 5 years ago

I submitted a fix some time ago, but I think @DefinitelyNotAGoat doesn't have much time right now.

RomarQ commented 5 years ago

Just use version 1.0.8 for now, it is stable.

DefinitelyNotAGoat commented 5 years ago

My wife and I just had a baby, so I've been absent. The environment file was a PR I merged and I probably shouldn't have. Libraries shouldn't have environment files, only applications should.

@RomarQ I see you closed your PR's. I really appreciate contributions, and I didn't get time to review. When I originally looked at them, there were like 6+ commits that needed to be squashed, and some git issues. I didn't have time to write out a peer review because of my newborn.

@tulpenhaendler, I removed the environment file and didn't realize the constructor was modified, I'll take a look.

DefinitelyNotAGoat commented 5 years ago

For the future, I'm going to set up unit tests, integration, format, vet, and staticcheck pipelines to be run and be required on any PR. A part of this refactor decision was to make a more test driven code base.

RomarQ commented 5 years ago

No problem, I closed the PR's since they got more conflicts from your last commit and didn't had time to fix those conflicts yet.

DefinitelyNotAGoat commented 5 years ago

@tulpenhaendler the constructor has been fixed. Sorry about your issues.