Rethink New function API

[UlisseMini]

Proof of concept implementation in #651

Problem

The New function does not express its intent in its type. it is also very unintuitive to use.

func New(args ...interface{}) (s *Session, err error)

The type signature tells the user nothing, i think this API was picked because of different ways to authenticate. I think this is a bad idea and it is more intuitive to use a struct.

Solution

Add a new more intuitive struct based API. something along the lines of this

dg, err := discordgo.WithAuth {
    Email: "foo@bar.com",
    Pass: "foobar",
    Token: "abc123",
    Bot: false,
}.Verify()

I'm using a method here to prevent me from having to type indiscordgo again. you could also do it like this

dg, err := discordgo.New(discordgo.WithAuth{
    Email: "foo@bar.com",
    Pass: "foobar",
    Token: "abc123",
    Bot: false,
})

In my opinion both are better then the current API, and could be added while remaining backwards compatible.

If this gets approved and i have time i'll implement this (it should not be that hard).

[iopred]

I like where this is heading, an API where you can pass in a WithAuth option, or WithToken option sounds great to me

…

On Sun, May 26, 2019, 7:25 AM Ulisse mini ***@***.***> wrote: Problem The New function does not express its intent in its type. it is also very unintuitive to use. func New(args ...interface{}) (s *Session, err error) The type signature tells the user nothing, i think this API was picked because of different ways to authenticate. I think this is a bad idea and it is more intuitive to use a struct. Solution Add a new more intuitive struct based API. something along the lines of this dg, err := discordgo.WithAuth { Email: ***@***.***", Pass: "foobar", Token: "abc123", }.Verify() I'm using a method here to prevent me from having to type indiscordgo again. you could also do it like this dg, err := discordgo.New(discordgo.WithAuth{ Email: ***@***.***", Pass: "foobar", Token: "abc123", }) In my opinion both are better then the current API, and could be added while remaining backwards compatible. If this gets approved and i have time i'll implement this (it should not be that hard). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#649?email_source=notifications&email_token=AALVLASQRO7JCOLPWYGQ5GTPXKMWRA5CNFSM4HPWY52KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GV4VRUQ>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALVLAVJJ36XO5VYXNSSGHTPXKMWRANCNFSM4HPWY52A> .

[diamondburned]

-1, You don't always need an email, a password and a token. I think the current system is fine as it is.

Luckily, since the current New function takes in an interface{}, you can add this without breaking, and I'm all up for that.

[UlisseMini]

@diamondburned If you don't specify a field then it will be set to its zero value, exactly what we want.

for example, not specifying the Bot field would leave it to a default of false
not specifying an email and password would leave them to "" and we could check for that in the New / Verify functions

[bwmarrin]

Personally, if we're to break the current API on everyone - I'm more in favor of dropping the Email/Pass entirely and just accepting a single token string that we pass along to Discord. It's a closer match to the goal of just being a wrapper around Discord's API and it's simple and easy to use.

The only reason this hasn't been done is because of not wanting to break the API on everyone for such a minor thing. Now with modules and versions and such, I do feel better about making breaking changes to help clean up this library.

[UlisseMini]

I agree with @bwmarrin, it would also make the API much simpler, something like

func New(token string) (*Session, error)

would be desirable

[ravener]

You could also remove the error return value entirely in that case, since currently it's only used for paramater validation and fetching token from email/pass, if it takes only token no error would be returned.

[diamondburned]

My PR should add something like this without any breaking changes: #725