-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Update API for Apple’s newly released API design guidelines #15
Comments
Guidelines are just that, and I like the changes and exceptions you have made. One that I am not sure about is Service.resource(url:) → resourceWithURL(_:) There is another guideline that says not to repeat type information. Which would make it: Service.resource(url:) → resourceWith(_: NSURL) I think the deprecation plan sounds good. |
Thanks for the review, Ray. You raise a good question about There’s value in having a flavor of the method that accepts a string instead of an NSURL, because it can fold “nil string” and “invalid URL” into a single method call. The logic for that is mildly tangled if there’s only an NSURL flavor of the method, and is likely to be repeated a lot. So you want one version of the method that takes a string that's a URL. However, there also needs to be a method that takes a string that’s a baseURL-relative path. At that point, there are two different
That’s not bad, but I feel like
service.resource("/widgets").child(widgetID).addObserver(…) But maybe I’m wrong about that, and the added clarity of
service.resourceWithPath("/widgets").child(widgetID).addObserver(…) |
I had forgotten about the “Compensate For Weak Type Information” section. Thanks for reminding me. I like the way resource("/user") looks too. Getting pedantic here but if you call it resourceWithPath I would naively think I need to specify the full path: e.g. One thing I was wondering about was resources always seem to with a forward slash. Is the shorter resource("user") frowned upon? |
Nah, I left it open to both styles on purpose. It doesn't matter, since that method always creates something underneath the base URL (which is meant to prevent security holes caused by, say, an accidental hostname switch). I tend to prefer |
Looking back over this list, I’m wondering about adding a prefix to the request hooks, maybe this: resource.load()
.onSuccess { data in print("Wow! Data!") }
.onFailure { error in print("Oh, bummer.") } …instead of this: resource.load()
.success { data in print("Wow! Data!") }
.failure { error in print("Oh, bummer.") } @rayfix, any others … gut reactions to that? |
+1 Generally I am a fan less typing, but this seems like a good change. The on prefix makes it 100% clear that a closure callback follows (rather than say returning a bool or error type). I also like that it groups both related operations together alphabetically and how that works with autocomplete. |
Good point about the autocomplete. Hadn’t even crossed my mind. I’ve updated the gist to reflect “on” prefixes as well. |
That's why it's a guideline, not a law! I do agree that this:
Seems best. The rationale being, (Having said that, I think it would be worth rethinking this particular API either way, because it's really not obvious that the
Again, I think Correct me if I'm wrong, but AFAICT the difference between I feel like this semantic difference isn't well conveyed in the naming, and the fact that "URL" doesn't just mean "NSURL", as there's an override that takes an equivalent URL string, confuses matters even more. What about
Agreed. I think the general rule is sound, but I think there's great value in this idiom as well, and the reason it works well is because it's followed by a closure. So it's going to look like this:
Which is super clear and nice. And in situations where you'd pass a reference to a method (not sure if applicable here), it's going to be clear too, as you'd generally make that an imperative verb phrase:
+1 on that as well. I'm wondering if a similar pattern wouldn't also work well here:
I think this is fine.
This is one of those cases where repeating type information might be preferable after all. And the reason why is that "URL" isn't just type information here, it also actually describes the thing the property is. And "base" works to describe "URL", not as a standalone noun. At first glance "Service.base" seems to suggest that there's a class called Base, or perhaps ServiceBase, or that there's some fundamental concept of a "base". And there isn't. There's just a base URL. |
Thanks for this in-depth review, Radek!
I like this as a counterpoint to the Apple guidelines. I’m still tentative about how I feel about that particular rule.
The cast becomes the narrower type of the
(It’s one of my favorite weird-awesome features of Swift that the inferred return type changes the runtime behavior of In fact, if there were to be a no-default version of this method — there isn’t, at least now, because it’s no more useful than
That line of thinking lead me to the current name.
You understand
If somebody tries to make your app leak sensitive information by making setting their username to The only place Siesta does relative resolution is in
I like your line of thought. Yeah, these methods have been a point of confusion for multiple users, and better naming might help. Since the distinction between the two methods is not absolute / relative, but rather URL / path, maybe this naming I shied away from is the right one after all:
Or:
@rayfix, any thoughts on this? Thanks for the sanity check on
Hmmm, you’re making me lean back to |
But that sounds weird too. "content as type"… yes, what type? Where's the type parameter? To my eyes, it looks incomplete. What about: It doesn't say much more, but there's no confusion. It just says "this is a typed version of the content", and you just about have to assume the type will be taken from the context this is passed to. You're right that when assigning to a variable it's not very useful, since And I think now
Okay, so I did get the point, just used the wrong words to describe the difference. But I would argue that "path" vs "url" is just as confusing. At least in the context of our community. All fault is on Cocoa, where people sort of learned that "path" is nothing else than a NSString version of NSURL (caveats apply). My reaction to this is "oh, so if I want to pass a string I use Obviously something super verbose like "resourceRelativeToBase" would be undesirable as well... Ideas?
Fair point! |
Thinking aloud here but what about Then change the current content() to untypedContent() or rawContent() (The or: is an idea I got from some API that Airspeed Velocity did recently. I think it looks nice and is still clear.) For the issue: You could define a simple Path type that better documents Path as something more akin to a Rails path type that gets appended to a base URL. |
Hmm, perhaps “path” is indeed the problem. How about “endpoint” as the other term?
That’s not perfectly correct, though, because of course external URLs can also be endpoints. Plus it loses the concision of the common case:
What about this?
On the typed content question, it's best to evaluate the method name in context, I think. One common case is defining an extension to provide a new convenience accessor — here, for example, one that lets you do
So there’s the problem: “as what type?” Like Radek said, it sounds like there’s a parameter missing — and that’s correct, in a way, because there is an implicit type parameter. With some of the alternatives:
This isn’t bad, but it's not perfect. It makes it sound like a resource can either have “typed content” or not, but all content is typed; it just may be of the wrong type. With the word “as” gone, it doesn’t convey in the same way the connection to Swift’s Ray’s suggestion:
I don’t think this conveys the type conversion nature of the operation enough. It’s a nice convention if we’re only defaulting for nil, but we’re defaulting for nil or wrong type. Does a preposition change or more verbosity fix it? I don’t think so, but throwing it out here:
Here’s another set of “in the wild” comparisons, for the common case of extracting a specific model type, and using Radek’s idea of the bare inferred type converter with no default value:
That touch off any opinions, or new ideas? |
Funnily enough, that's exactly what I suggested a few comments before :) (albeit admittedly didn't make it this explicit). So yes, +1. I'd also be willing to consider dropping the "absolute" word, and just have
Good point, haven't thought of that. I think the reason why I'm drawn to the And when it starts being a pattern, seeing "foo" and "typedFoo" pairs in many places becomes a nice convention, and has a nice symmetry. Out of these:
I still like
EDIT: "content" would be great (and, if needed for whatever reason, "rawContent" for untyped version) if only the type was explicit or unambiguous somehow. But since it's inferred from the context, I think it would only bring confusion. |
Ha! That is exactly my original design! Full circle. I also thought it was clear, but it was a point of confusion for at least a couple of different people. I’d tentatively lean toward this, then, since we both like it:
Ray, others, gut check on that?
Funny thing: that was my original name for it. I switched to I still like these two best, and definitely agree with Radek’s analysis of the others:
I could be talked back into |
Looks good to me.
I slightly prefer typedContent. Seems just as clear if not more clear than the first one (first looks like it should have an arg) and also is less typing. :haha: |
I’ve locally gone back to I’ve also experimented with going back to
…is more pleasant than:
So … yeah, I think you’ve won me over. If I still like it by the end of the weekend, it shall be so. On reflection, I’m liking the new use of “absolute” in the method name, but am on the fence about this:
…versus this:
Like you all, I find the first one appealing, but it does clearly violate Apple’s guidelines as well as common practice. @erica, any chance I can nudge you into a gut reaction on this one? Seems like the kind of call you’re good at making. |
I'm not so sure about this use case (I tend to avoid explicit type declarations inside methods like that), but one that I find compelling is when the type can be inferred from a method you're passing typedContent() to.
The common practice is based on the fact that Objective-C doesn't really have the concept of argument labels the same way Swift does, rather, it's all part of a single the method name. So you have a common pattern of Notice how Objc->Swift bridging does a better job at translating
becomes:
Because it simply makes more sense. "Name" describes the parameter, it's not really a part of the method name itself. As noted before, I also disagree with you saying that this "clearly" violates guidelines. Again:
Note "usually". The guidelines note a few exceptions, but I would argue the list is incomplete, and this is a good use case where it makes more sense to use the explicit argument label rather than force it to be part of the name. Also, don't forget about:
Why say "with" in the name, when separating "absoluteURL" into a param name conveys the same semantics (and arguably, better)… |
Sure, you could also do this nice refactoring:
You’d then have to make
…but that’s probably the right thing anyway, since “show that there is no user” is also a valid & important operation. And I still do like OK, Radek, I think you’ve convinced me on I say “clearly violates” because after that “usually” you mention, the Apple guidelines then say, “There are only a few exceptions,” and this is not among those they list. However … I don’t see any examples in the guidelines that clearly promote a first arg label to be part of the method name. The closest they come is this:
…but in this case,
That’s “can” and not “should,” but it does suggest we’re not out of line thinking that method base names are about the method’s role, and not its first argument. |
I'm not ignoring you as style is pretty much at the top of my interest list right now, it's just that I'm dealing with one ridiculous family drama after another. Last kid goes back to school on Thursday. I'm having big issues with the current minimized APIs in that you must quick-look the apis often to see the name of the parameter label, which is not a reasonable thing to do when you're trying to read code. Apple's guidelines are squooshing together usecases: first, is implementation, where you see the entire signature (their primary design goal) but they're not properly serving the more common use-case: reading and reviewing. Remember: code is written once and read often, so readability should focus on serving the latter over the former and I'm not sure their guidance s proper here. There's the older style API guides that would suggest: resourceWithAbsoluteURL("https://other.server/dingbats https://other.server/dingbats") It's clear, its immediately understandable, and it's a mouthful. vs this which is not resource("https://other.server/dingbats https://other.server/dingbats") This would be way better but it violates style with its initial label, use of type, etc. resource(absoluteURL: "https://other.server/dingbats https://other.server/dingbats") Guidance says:
so I suppose the "suggested" API would be: absoluteResource("http://other.server/dingbats http://other.server/dingbats") Which I don't love -- E
|
Thanks, Erica. I didn’t think you were ignoring us! Certainly nobody’s obliged to respond to a random Github mention, especially on the weekend, so I’m delighted to have you jump in at all. To give some background, because I know this is a long thread to jump into: there are several different One variant takes a subpath of a service’s base URL; the other takes a fully independent URL, and that latter one comes in two flavors that accept String and NSURL. So there’s both a “path vs. URL” distinction and an “under the base URL vs. absolutely nothing to do with base URL” distinction. I lean (as I think do Ray & Radek) toward making the more common “subpath of base URL” variant be unlabeled, because with the strings you’d actually be passing to that, it really is pretty clear at the call site:
…whereas more explicit labels like this get ugly, especially given that they’re such a common case:
(end of background) I totally agree that this is not good:
…because, among other reasons, there’s no such thing as an “absolute resource.” Every resource has a fully resolved URL; the only difference between all these When you say that “this would be way better:”
…you are in agreement with everyone else in this thread. It passes my own gut check, and three other developers whose opinions I respect a lot say it looks good to them. That’s enough for me to go against the Apple guidelines. If we do get a chance to comment on the API guidelines on swift-evolution, then we can make our stand there! Get your torches and pitchforks ready, I guess? |
I think what we need to do is start writing some proper API guidelines that aren't limited to or just a response to the ones currently on offer (set up a repo?), start hitting them with real world examples like siesta, and then submit our guidelines once Apple opens up the API guides to public review. -- E
|
If I you set up the repo, I’ll definitely drop in my $0.02! FWIW, I really like the new guidelines in a great many respects. They’re just too prescriptive given the narrow range of cases they actually consider — particularly around part-of-speech questions and parameter naming. |
Awesome! Great discussion, and thanks @erica for chiming in!
Same. I think the guidelines have room for refinements, and if there's something we can do as a community to show examples where seemingly the best option goes against the guidelines, then we should do it. Let's just not get hung up on following the guidelines 100%. It's "guidelines", not "the law" — the word itself suggests that it describes the 90% or maybe 95% cases, but there's always room for good judgement for the edge cases. I'm more interested in the spirit of the guidelines, not the letter. |
I reviewed Siesta’s API against Apple’s new API Design Guidelines for Swift, and identified ~27 names of concern. Of those, I recommend changing
814.Details of my review, proposed changes, and rationale are in this gist: https://gist.github.com/pcantrell/22a6564ca7d22789315b
If anybody has opinions about these changes, or notices other places where Siesta’s naming conventions contradict the new guidelines, please comment on this issue.
API changes will appear as deprecations in beta 6, and the deprecated methods will be removed in the subsequent release.
The text was updated successfully, but these errors were encountered: