Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sling.Safely() #4

Closed
wants to merge 1 commit into from
Closed

Add Sling.Safely() #4

wants to merge 1 commit into from

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented Apr 30, 2015

When safe mode is enabled, all Sling methods copy the underlying
sling instance before mutating the Sling object effectively making
Sling immutable.

This is just a proof of concept to see whether it makes sense. It needs more
documentation and testing before it's shippable, but I wanted to get feedback
before doing that work.

I think ultimately this mode would be inverted as 'unsafe mode' so that we do
the least surprising thing by default and allow clients to opt-in to mutable state,
but I've erred on the side of not changing existing behavior.

When safe mode is enabled, all Sling methods copy the underlying
sling instance before mutating the Sling object effectively making
Sling immutable.

This is just a proof of concept to see whether it makes sense.  It needs more
documentation and testing before it's shippable, but I wanted to get feedback
before doing that work.

I think ultimately this mode with be inverted as unsafe mode so that we do the
least surprising thing by default and allow clients to opt-in to a mutable
world, but I've erred on the side of not changing existing behavior.
@dghubble
Copy link
Owner

Thanks for the offline note about this Sling mutability issue and this clever approach.

For others, a Sling is effectively a generator for a kind of http.Request (say with url x, JSON data y) so any setters will mutate the builder. The final "built" output is the http.Request from calling mysling.Request().

I think this is what most users expect in the simple case. Where it can get tricky is when trying to make 2 or more Slings which extend from a common one.

base := sling.New().Base("http://example.com/").Client(client)
a := base.Base("http://test.com/")
b := base.Path("/foo")

At first glance, it might look like there are two Slings here, a and b, but, chained setters just return the same builder. The 2nd line sets the Base again, overriding http://example.com. The Path call just extends the path as you'd expect. The result is that both a.Request() and b.Request() make a request to "http://test.com/foo".

To correctly make a new Sling which copies properties from an existing Sling, there is a New() method which will create a completely independent copy.

base := sling.New().Base("http://example.com/").Client(client)
a := base.New().Base("http://test.com/")
b := base..New().Path("/foo")

So a.Request() is to "http://test.com/" and b.Request() is to "http://example.com/foo" and both use client.

@olix0r Using New() copying inside every mutation ensures every setter returns a completely new Sling (effective immutability as you say), but I wonder if a doc improvement might accomplish the same? Copying at every step seems heavy weight to me. I'd prefer users be cognizant of the fact that a copy is needed to extend a Sling, otherwise you're mutating the parent one. The way Template sets are built in the html/template package and an explicit Clone method is provided strikes me as similar to how Sling handles extensions.

For #7 I'd like to:

  • Improve Sling docs about copy extension and place it later in the README #700ae3d45b
  • Internalize all Sling fields so all mutations are occuring through the setters, which is more predictable

@olix0r
Copy link
Contributor Author

olix0r commented May 22, 2015

The specific case that I found confusing was something like the following:

foo := new(Foo)
bar := sling.New().Base("http://bar.com/").Client(client)
for ... {
    bar.Get("/foo").Receive(foo)
    ...
}

which leads to the following requests:

0: bar.com/foo
1: bar.com/foo/foo
2: bar.com/foo/foo/foo
3: bar.com/foo/foo/foo/foo
...

Documentation will help things, though I think that it is very easy to accidentally write a broken client (like above)--there's no situation where doing sling.New().Get("/foo").Get("/foo") makes sense with the current semantics.. The thrust of my argument for automatically cloning is that it then becomes exceedingly difficult (impossible?) to do the Wrong Thing. I'm really not attached to this, since I understand "the trick" to sling now, but I'd encourage you to not put too much faith in people reading the docs ;)

@dghubble dghubble closed this May 26, 2015
@olix0r olix0r deleted the optionally-safe branch June 5, 2015 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants