schmorrison / Zoho

Golang API for Zoho services
MIT License
35 stars 34 forks source link

Export crm.crmModule #23

Closed rollulus closed 3 years ago

rollulus commented 3 years ago

Hi, thanks for your work.

I'm using the crm module, and I would like to define an interface for some of crm.API's methods, e.g.:

type x interface {
    InsertRecords(request crm.InsertRecordsData, module crm.crmModule) (data crm.InsertRecordsResponse, err error)
}

var _ x = &crm.API{}

However, crm.crmModule is not exported, so it's not possible (as far as I know) to define such an interface as a package user. And therefore, dependency injection is impossible, and testing becomes more complicated than needed.

Hence, my PR, in which crm.CRMModule gets exported. Sidenote: I think it's better to name it crm.Module to avoid the stutter, but I didn't want to make a too disruptive change. Thanks!

schmorrison commented 3 years ago

I am concerned about breaking anyones projects. Ideally they could be using go mod now, are there any alternatives? Could we simply make crm.crmModule an alias of string, eg. type crmModule = string ? Would the compiler survive that?

I know this kind of thing works if everything is in main: play

schmorrison commented 3 years ago

I agree with the stutter. The type was originally in the Zoho package before I broke it into a subpackage, unfortunately some of my names weren't changed when I did.

schmorrison commented 3 years ago

Okey dokey, had a chance to give it a try. It definitely compiles by making the type an alias type crmModule = string then using string as the type in the interfaces call signature.

Its probably the simplest and least distruptive way to make this change. I haven't actually tested that the interface doesn't have any runtime problems. I encourage you to give it a try.

githhub.com/schmorrison/zoho/crm/crm.go

type crmModule = string

main.go

type x interface {
    InsertRecords(request crm.InsertRecordsData, module string) (data crm.InsertRecordsResponse, err error)
}

var _ x = &crm.API{}
rollulus commented 3 years ago

Hey thanks! I’m on holiday right now, will come back to you next week.

On 20 Oct 2020, at 17:35, Sam Morrison notifications@github.com wrote:

 Okey dokey, had a chance to give it a try. It definitely compiles by making the type an alias type crmModule = string then using string as the type in the interfaces call signature.

Its probably the simplest and least distruptive way to make this change. I haven't actually tested that the interface doesn't have any runtime problems. I encourage you to give it a try.

githhub.com/schmorrison/zoho/crm/crm.go

type crmModule = string main.go

type x interface { InsertRecords(request crm.InsertRecordsData, module string) (data crm.InsertRecordsResponse, err error) }

var _ x = &crm.API{} — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

rollulus commented 3 years ago

I'm back. What I do not understand is: in what scenario will exporting crm.crmModule break existing code? I believe I'm missing something, and something will break indeed, citing the Go blog: "There is, in fact, no backward-compatible change you can make to a function’s signature". But what will break exactly here?

schmorrison commented 3 years ago

Well that's logical. Never considered that no one could possibly be using any of the call signatures as they exist.

Make the change to crm.Module. I'll version during the merge.

Best regards,

Sam Morrison

On Mon., Oct. 26, 2020, 1:45 a.m. Rollulus Rouloul, < notifications@github.com> wrote:

I'm back. What I do not understand is: in what scenario will exporting crm.crmModule break existing code? I believe I'm missing something, and something will break indeed, citing the Go blog https://blog.golang.org/module-compatibility: "There is, in fact, no backward-compatible change you can make to a function’s signature". But what will break exactly here?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/schmorrison/Zoho/pull/23#issuecomment-716372352, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEEUOIYBSTKBTQKQEAHUTH3SMUSJ5ANCNFSM4SRTJWPQ .

schmorrison commented 3 years ago

Any updates on this pal?

rollulus commented 3 years ago

Apologies, completely forgot. Thanks for the reminder.