adlio / trello

Trello API wrapper for Go
MIT License
219 stars 71 forks source link

Use `args ...Arguments` instead of `args Arguments`? #74

Closed TJM closed 3 years ago

TJM commented 3 years ago

Hi, I found your module when trying to "publish" TJM/go-trello (fork from VojtechVitek/go-trello ) that I hacked on for several days. I like a lot of the things you have done here, and I am ready to switch over. I was actually working on switching my (real) project over to use your module instead and it occurred to me...

Couldn't you swap all the args Arguments for args ...Arguments, thus making the "trello.Defaults()" on every call unnecessary (optional)? It may not be exactly that, but something like that? The go-trello code that I "forked" had Argument as a struct with Name and Value, then []Arguments or ...Argument :-/ ... just checking as that feels a bit strange to generate a new/empty map for every call. At the very least, I am thinking create it once at the top and use a variable for each one? 🤷‍♂️

I am fairly new to golang, so I am not sure what the ramifications of that are?

Also, Based on my findings trying to hack go-trello into submission, I think I can offer several PRs to add capability/functionality if you are interested.

One of the biggest epiphanies I had was the "AddMemberResponse" is actually a (partial) "Board" response... of course you have to have []Memberships and []Members in your Board type. Anyhow, lets focus on the "optionalization" (new word?) of the args for this issue. :)

~tommy

EDIT: I finally found the right word "variadic" :)

https://www.geeksforgeeks.org/golang-program-that-uses-func-with-variable-argument-list/

At the most basic level, you could just use args ...Arguments and then add a [0] to the places where you access args now, but of course the ... might encourage people to pass more than one list of arguments, so you should maybe come up with some way to "flatten" them? It seems like you already had some "manual" flattening going on (like "if pos, then set pos")... so it would already be useful IMO.

TJM commented 3 years ago

As noted, check my PR #78 for the "fix" for this.