Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333163003:
Add Uri.replace which creates a new Uri with the same fields as the original, but with some fields … (Closed)
|
Created:
6 years, 6 months ago by Lasse Reichstein Nielsen Modified:
6 years, 4 months ago CC:
reviews_dartlang.org, floitsch, kustermann Visibility:
Public. |
DescriptionAdd Uri.replace which creates a new Uri with the same fields as the original, but with some fields replaced by the user.
BUG= http://dartbug.com/18950
R=ajohnsen@google.com
Committed: https://code.google.com/p/dart/source/detail?r=38894
Committed: https://code.google.com/p/dart/source/detail?r=38897
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated documementation slightly #Patch Set 3 : Reapply after revert #Patch Set 4 : Update to work with new Uri internal structure #
Total comments: 2
Patch Set 5 : Address comment. #Messages
Total messages: 21 (0 generated)
DBC. https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, I don't like "replace" it gives the impression that it manipulates the receiver. maybe "with" or "withReplaced" or "toUriWith" or "toUri" ?
https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, I picked "replace" because we already use it in the same sense on String. "with" is a keyword, otherwise it would be my preferred alternative. I don't like the other suggestions. Too long without actually being better, but I would be open to another, still simple and telling, name.
https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, On 2014/06/24 09:25:58, Lasse Reichstein Nielsen wrote: > I picked "replace" because we already use it in the same sense on String. > "with" is a keyword, otherwise it would be my preferred alternative. Could we push to make `with` a pseudo-keyword? But true: we use a similar name on String, too. > > I don't like the other suggestions. Too long without actually being better, but > I would be open to another, still simple and telling, name.
+kevmoo DBC https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, On 2014/06/24 09:32:27, floitsch wrote: > On 2014/06/24 09:25:58, Lasse Reichstein Nielsen wrote: > > I picked "replace" because we already use it in the same sense on String. > > "with" is a keyword, otherwise it would be my preferred alternative. > Could we push to make `with` a pseudo-keyword? > But true: we use a similar name on String, too. > > > > I don't like the other suggestions. Too long without actually being better, > but > > I would be open to another, still simple and telling, name. The Uri class is only one example. I've argued with kevmoo that it is nearly always beneficial to have copy-on-write functions on immutable data types (out of this discussion he filed the bug). The shelf package has now immutable (http) Request/Response classes and they use the name "change()". I have no strong preferences here, but we should be consistent I think. It may also be worthwhile to mention that it is not possible to set a field to "null" when it was non-null before using this method.
https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, I like "change" even less than "replace", when it comes to not sounding like it changes the object. Another alternative is "update", with all the same weaknesses. I'd go for replace, for consistency with String, if not for anything else. Another alternative is "where", but we already used that for a completely different thing. Sadly "with" is taken as a keyword. Now, if we allowed arbitrary parameters to operator[], we could do: var uri2 = uri[path: "/asafss/", query: "nix"]; THAT would be neat. We could use "call" to make it callable and do: var uri2 = uri(path: "/asafss/", query: "nix"); Not as good, though, and probably very confusing to read. Good point on the inability to replace something with nothing. You'll have to use the Uri constructor manually in that case.
https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, What about Uri.from(Uri uri, {...}); ?
> What about
>
> Uri.from(Uri uri, {...});
My only complain is that it's longer and less readable. Instead of
x.replace(arglebargle)
it's
new Uri.from(x, arglebargle)
Not a LOT longer, but longer, and harder to type (with a space and all :).
It's extremely explicit that a new Uri is created, and it matches our other
.from-constructors in creating something from something else, optionally with
modifications, like List.from(list, growable: false).
On the other hand, we also have List.toList(growable: false), which I prefer to
use.
I kind of like it, but would prefer an inline method name.
How about toUri?
A good thing about making it a constructor, is that it can easily be tree-shaken, if it's not used. That way we don't have to think about the added code size. That being said, I understand your concern. But 'change' and 'replace' are really bad names IMO, as it indicates that it's actually mutating the object, not creating a new object - you know, like calling a constructor ;) If we can find a name that is less misguiding, I'd be happy with this CL.
I'm warming to the constructor solution, but there is one small problem with it. With a method, the method knows the class it is on, and can avoid validating the fields that are not copied. A constructor/static method cannot know whether its argument is an actual Uri or some other class implementing Uri, so it should validate all the fields it reads off the other Uri.
On 2014/06/26 12:15:50, Lasse Reichstein Nielsen wrote: > I'm warming to the constructor solution, but there is one small problem with it. > > With a method, the method knows the class it is on, and can avoid validating the > fields that are not copied. A constructor/static method cannot know whether its > argument is an actual Uri or some other class implementing Uri, so it should > validate all the fields it reads off the other Uri. copy(...) or copyWithChanges(...) ?
> A good thing about making it a constructor, is that it can easily be > tree-shaken, if it's not used. That way we don't have to think about the added code size. What makes you think you cannot shake the method away if it's not used? The named parameters would need to match as well as the method name, I guess. I don't think there will be a lot of change/replace methods with the same named parameter names. Though, I'm not an expert in dart2js type inferencing. But it is a valid point. See below what you gain by having it as an instance method. > That being said, I understand your concern. But 'change' and 'replace' are > really bad names IMO, as it indicates that it's actually mutating the object, I have never heard you complain about String.replace(). It's basically the same thing. It is replacing/changing something, it just makes a copy before doing so. > not creating a new object - you know, like calling a constructor ;) The "new" keyword is IMHO horrible. In c++ days it might have made sense, because you knew when you're allocating an object on the heap. But nowadays you create objects all the time without knowing it -- closures for example. I think factory constructors are even counter-intuitive: they might always return the same object and not allocating one (even though you have "new"). > I'm warming to the constructor solution, but there is one small problem with it. > With a method, the method knows the class it is on, and can avoid validating the > fields that are not copied. A constructor/static method cannot know whether its > argument is an actual Uri or some other class implementing Uri, so it should > validate all the fields it reads off the other Uri. Very good point. Additionally you have one very nice thing (I've argued that as well in the http Request/Response case -- less relevant for Uri, but if we think about immutability+copy-on-write in general): Imagine you have "class Base" and "class SubBase extends Base": - Users of variables with "Base" can call change() and the polymorphic "SubBase.change()" function can create another "SubBase" - If you made a constructor call, then the user would not know about "SubBase", so he creates a "new Base()" => This allows you to pass immutable objects around to code, which changes these objects in a COW way, and knows only about a base type, and gives the changed version back to you, and you still have the subclass. Very nice IMHO. [but as I said, less relevant for Uri]. https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, On 2014/06/26 08:20:35, Anders Johnsen wrote: > What about > > Uri.from(Uri uri, {...}); > > ? It's longer, not nice and will be special for Uri. I would rather like have a very nice mechanism of changing immutable data types in a copy-on-write fashion. https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:826: Uri replace({String scheme, Why do you have so many concerns about replace/change/... ? It is really replacing/changing something, right? It's not replacing it in-place but rather makes first a copy. I think this is fine. If we make the pattern consistent, people will easily get it. Have you ever heard a single complaint about "String.replace()" ? I did not. On 2014/06/26 06:01:31, Lasse Reichstein Nielsen wrote: > I like "change" even less than "replace", when it comes to not sounding like it > changes the object. Another alternative is "update", with all the same > weaknesses. I'd go for replace, for consistency with String, if not for anything > else. > > Another alternative is "where", but we already used that for a completely > different thing. Sadly "with" is taken as a keyword. > > Now, if we allowed arbitrary parameters to operator[], we could do: > var uri2 = uri[path: "/asafss/", query: "nix"]; > THAT would be neat. > We could use "call" to make it callable and do: > var uri2 = uri(path: "/asafss/", query: "nix"); > Not as good, though, and probably very confusing to read. > > > Good point on the inability to replace something with nothing. You'll have to > use the Uri constructor manually in that case.
message: On 2014/06/27 20:45:06, kustermann wrote: ... > The "new" keyword is IMHO horrible. http://dartbug.com/18241 > In c++ days it might have made sense, > because you knew when you're allocating an object on the heap. C++ needed it because it could allocate objects inline or on the heap, and it only used it for the heap version. The rest of us are just parroting that without any real reason. ... > Imagine you have "class Base" and "class SubBase extends Base": > - Users of variables with "Base" can call change() and the polymorphic > "SubBase.change()" function can create another "SubBase" > - If you made a constructor call, then the user would not know about "SubBase", > so he creates a "new Base()" Constructors taking magical values (e.g., assuming a specific implementation of the argument type) is a design smell. Typed lists have that problem. I had a CL at some point where ByteBuffer had methods for creating views (createUint8List), so the Uint8List didn't have to have a magical link to ByteBuffer in its view constructor. I should see if I can resurrect that. > => This allows you to pass immutable objects around to code, which changes these > objects in a COW way, and knows only about a base type, and gives the changed > version back to you, and you still have the subclass. > Very nice IMHO. > > [but as I said, less relevant for Uri]. For now. But what if we had a FileUri class with extra file-related functionality, and our Uri constructors returned that if the scheme was "file"? That's not even a bad idea. ... > Have you ever heard a single complaint about "String.replace()" ? > I did not. Not here, but it's just a matter of time: https://encrypted.google.com/search?q=string+replace+doesn%27t+change+the+string I can live with replace. I can't find a better alternative ("copy" has potential, but lacks something, and "copyAndModify" is too long and cumbersome).
Ok, not better suggestions have surfaced. Unless Luke has a better idea, let's use "replace". PTAL.
lgtm, but with the same concerns about the name 'replace' :) https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:808: * Using this method is similar to using the `new Uri` constructor with This is the second paragraph mentioning it's similar to `new Uri`.
https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/1/sdk/lib/core/uri.dart#newcod... sdk/lib/core/uri.dart:808: * Using this method is similar to using the `new Uri` constructor with I think it's ok. It really is similar. I've slightly rewritten the documentation.
Message was sent while issue was closed.
Committed patchset #2 manually as 38894 (presubmit successful).
Updated code, PTAL
lgtm https://codereview.chromium.org/333163003/diff/60001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/60001/sdk/lib/core/uri.dart#ne... sdk/lib/core/uri.dart:887: if (schemeChanged) { Add comment. Also, use else?
https://codereview.chromium.org/333163003/diff/60001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/333163003/diff/60001/sdk/lib/core/uri.dart#ne... sdk/lib/core/uri.dart:887: if (schemeChanged) { Comment added. Can't see any use for "else" here.
Message was sent while issue was closed.
Committed patchset #5 manually as 38897 (presubmit successful). |
