hudl / fargo

Golang client for Netflix Eureka
MIT License
133 stars 53 forks source link

Questions about EurekaConnection.generateURL() #9

Open cquinn opened 10 years ago

cquinn commented 10 years ago

It seems like generateURL() could be use to completely assemble the URL from its slug parts just by passing the parts as varargs. But none of the calling code actually uses this feature, and instead assembles most of the URL with a couple different approaches.

E.g. in RegisterInstance(), two approaches in combination are used, Sprintf and later string cat:

slug := fmt.Sprintf("%s/%s", EurekaURLSlugs["Apps"], ins.App)
reqURL := e.generateURL(slug)
_, rcode, err := getBody(reqURL+"/"+ins.HostName, e.UseJson)

And it seems that using generateURL() to do the work would be simpler:

reqURL := e.generateURL(EurekaURLSlugs["Apps"], ins.App, ins.HostName)

Second question: what purpose does the EurekaURLSlugs[] lookup serve if it only ever maps a string to a nearly identical string. Maybe we can just remove it? Then the above would be:

reqURL := e.generateURL("apps", ins.App, ins.HostName)

I can test this and prepare a PR if it makes sense to you.

tysonstewart commented 10 years ago

These changes make sense to me. I'm not sure if @ryansb has any objections, though.

ryansb commented 10 years ago

Oops, I read and (thought) I'd :+1:'d this already.

In any case, sounds great.