search

jagregory / halgo

HAL implementation in Go
MIT License
33 stars 9 forks source link

navigator is not exported #11

Open xogeny opened 9 years ago

xogeny commented 9 years ago

When writing code, it is nice to be able to write functions that work with navigator instances. This might involve taking them as arguments, returning, etc. This comes up occasionally when writing testing code or trying to build higher-level functions on top of a hypermedia API.

However, the fact that navigator is not exported makes this impossible (as far as I can tell). I'm not sure what the rationale is behind this, but it is quite frustrating. Would it be possible to change this?

jagregory commented 9 years ago

Pull request welcome. This library was developed for my own purposes, the "rationale behind this" is I never needed to expose it so I didn't expose it. Happy for that to be changed though.

jhiemer commented 9 years ago

@xogeny I totally agree with you. Did you change that in your fork?

xogeny commented 9 years ago

@jhiemer I haven't done anything with my fork regarding this issue. It is a simple fix.

The real issue is that creation of a navigator instance involves the use of a function called Navigator. As such, it isn't a simple matter of renaming navigator to Navigator. This is, unfortunately, non-idiomatic Go. If it were idiomatic, the function would be named MakeNavigator (since it returns a non-pointer). If the idiomatic conventions were followed, then the rename would be simple.

So the bottom line is...what should navigator be renamed to? Since it was previously not exported, nothing (external) can be depending on the old name. But we can't just capitalize the first letter since the function Navigator already exists.

I see two options. Make halgo idiomatic and change navigator to Navigator and Navigator to MakeNavigator. That will (of course) break the API.

The other option is to keep Navigator as it is and change navigator to something that is at least exportable, e.g., HalgoNavigator or something.

Personally, I really prefer the API-breaking approach because I feel it gets things "on track". Perhaps this could be considered in conjunction to a move to gopkg.in so that the API breaking version could be recognized as a distinct version from previous ones?

jhiemer commented 9 years ago

@xogeny thanks for this really enlightening explanation. I tried to "quick fix" what you said in the morning, but ran exactly into the same issue.

Currently I am not really aware of the process behind API breaks in Go, as I started the usage of the halgo project as part of my learning with Go itself. So far what I read & understand, I would agree with you change.

jagregory commented 9 years ago

I'm happy to take on a breaking change. Halgo is still evolving, I don't mind breaking changes to get it on track with proper conventions.

I'm less comfortable with moving to gopkg.in, mainly because I don't know enough about it. I'd prefer to consider these two separate issues.

On Tue, Mar 31, 2015 at 1:36 AM, Johannes Hiemer notifications@github.com wrote:

@xogeny https://github.com/xogeny thanks for this really enlightening explanation. I tried to "quick fix" what you said in the morning, but ran exactly into the same issue.

Currently I am not really aware of the process behind API breaks in Go, as I started the usage of the halgo project as part of my learning with Go itself. So far what I read & understand, I would agree with you change.

— Reply to this email directly or view it on GitHub https://github.com/jagregory/halgo/issues/11#issuecomment-87704391.

James Gregory

Tel: +61 (0) 411 619 513 Website: http://jagregory.com Twitter: @jagregory http://twitter.com/jagregory

jhiemer commented 9 years ago

Another question I have here is, if would make sense to implement the Navigator as a Singleton?

xogeny commented 9 years ago

No, I don't think so. I can easily imagine (in my use cases) having to have different instances of a Navigator where each is talking to the root of a different API. But there is even another use case...when you do a POST, you (should) get back a response that includes a Location header. That is the location of the newly created resource. As such, you won't know how to navigate to that from the API root (i.e., the existing Navigator instance). So it is simplest to just create a new Navigator instance using the new resources URI as the entry point.

Bottom line...I think forcing a singleton pattern would rule out lots of important scenarios.

jhiemer commented 9 years ago

You are right. It was a bit short sighted. The main benefit I thought would be to define the Headers (Auth-Token, Content-Type, Accept etc.) once for the whole application and being able to work on this definition during the whole application lifetime throughout different files, without having to redefine it.

Currently this is bound the instance of the navigator, if I understand the code correctly. Or?

xogeny commented 9 years ago

You are right. But I don't think that is a problem. Each operation on a Navigator creates a new instance. As such, there is no "cross talk" between them. So it is entirely reasonable and possible with the current implementation to create a single instance of Navigator and use it to spawn all you requests. As such, you can specify the session wide attributes just once on that single instance and they should get inherited by all subsequent copies that are made as a result of chaining.