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

Rename clients.getAll() #610

Closed
jakearchibald opened this issue Jan 22, 2015 · 11 comments
Closed

Rename clients.getAll() #610

jakearchibald opened this issue Jan 22, 2015 · 11 comments

Comments

@jakearchibald
Copy link
Contributor

…since it has filtering options that filter by default.

@annevk suggested .get(), but does that suggest a singular return?

Could be .matchAll() like the caches.

@jungkees
Copy link
Collaborator

.get() sounds more like getting a singular result.
.getAll(options) still LGTM as it suggests "get all the clients which meet the options which are happen to be controlled window by default for convenience. (It can get actually all.)"
But in case people still think it's not natural, .matchAll() seems fine to me too.

@KenjiBaheux KenjiBaheux added this to the Version 2 milestone Jan 23, 2015
@KenjiBaheux
Copy link
Collaborator

Proactively filed http://crbug.com/451334 for Blink, pending a decision.

@jakearchibald
Copy link
Contributor Author

@annevk still unhappy with getAll? Other suggestions are matchAll, queryAll.

@annevk
Copy link
Member

annevk commented Jan 27, 2015

I don't see how All makes sense given that it doesn't return all of them by default. Also, none of those names signifies that you are getting back copies rather than references.

@mvano
Copy link

mvano commented Jan 27, 2015

How about just filter or match?

@slightlyoff
Copy link
Contributor

@mvano: would we need to pun filter with Array.prototype.filter?

I'll go with whatever this group likes, but @mvano's match and .matchAll() as previously suggested sound good to me. They're at least internally consistent with the identity semantics that @annevk is worried about.

@mvano
Copy link

mvano commented Jan 28, 2015

would we need to pun filter with Array.prototype.filter?

Not sure what pun means in this context, but I recognize the potential confusion of having a method with the same name but different functionality on different classes. The same issue exists with String.prototype.match. I don't find this a major issue though. In an object oriented design you have natural namespacing and no real conflict.

@inexorabletash
Copy link
Member

Since we're bikeshedding... how about getClients() ?

Yeah, self.clients.getClients() is a bit weird. But it does exactly what it says on the tin.

@jakearchibald
Copy link
Contributor Author

Lets go with matchAll. Matches caches.matchAll which has a similar arg with filtering rules.

@wanderview
Copy link
Member

Tracked in gecko here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1130685

@ZhuoyuQian
Copy link

Commit a CL here:
https://codereview.chromium.org/892053003/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants