View mode: basic / threaded / horizontal-split · Log in · Help
October 25, 2012
Re: Uri class and parser
On 2012-10-25 13:24, Mike van Dongen wrote:

> Now it allows you to create/edit an URI. You can do so by using an array
> or string, whichever you prefer.
> I also added a toString() method and fixed the indentation to 4 spaces,
> instead of 1 tab.
>
> uri = new Uri();
> uri.scheme = "foo";
> uri.username = "username";
> uri.password = "password";
> uri.host = "example.com";
> uri.port = 8042;
> uri.path = ["over", "there", "index.dtb"];
> uri.query = ["type": "animal", "name": "narwhal", "novalue": ""];
> uri.fragment = "nose";
> assert(uri.toString() ==
> "foo://username:password@example.com:8042/over/there/index.dtb?novalue=&name=narwhal&type=animal#nose");

Awesome, now it's only missing documentation :)

-- 
/Jacob Carlborg
October 25, 2012
Re: Uri class and parser
On Wednesday, 24 October 2012 at 12:47:15 UTC, Adam D. Ruppe 
wrote:
> BTW don't forget that this is legal:
>
> ?value&value=1&value=2
>
> The appropriate type for the AA is
>
> string[][string]

Thanks for the type! It's now implemented.

uri = Uri.parse("http://dlang.org/?value&value=1&value=2");
assert(uri.queryMulti == ["value": ["", "1", "2"]]);
assert(uri.query["value"] == "2");


On Thursday, 25 October 2012 at 11:44:11 UTC, Jacob Carlborg 
wrote:
> Awesome, now it's only missing documentation :)

It's a dirty job, but someone's gotta do it ;)
I have commented all code that's not straightforward, but nothing 
for the Ddoc.

Can you give me an example of how specific I need to be in the 
documentation?


On Wednesday, 24 October 2012 at 19:54:54 UTC, Jacob Carlborg 
wrote:
> On 2012-10-24 20:22, Mike van Dongen wrote:
>> As all my code is part of a single class and the file 
>> std/uri.d already
>> existed, I decided to 'just' append my code to the file. 
>> Should I
>> perhaps put it in another file as the private methods you 
>> mentioned are
>> not relevant to my code?
>
> If the some methods aren't used by the URI parser you should 
> remove the. If they're used I would suggested you move the 
> further down in the code, possibly at the bottom.

It's not clear to me what you mean by this.
To clarify: the first 520 lines weren't written by me, and the 
code I have written doesn't use any of those functions.
Atleast, for now; Moving the functions 'encode' and 'decode' into 
the class Uri may be useful at a later point.

As I'm the new kid on the block, I'm trying not to break others' 
code. ;)
October 25, 2012
Re: Uri class and parser
Mike van Dongen wrote:
> Hi all!
> 
> I've been working on an URI parser which takes a string and then
> separates the parts and puts them in the correct properties.
> If a valid URI was provided, the (static) parser will return an
> instance of Uri.
> 
> I've commented all relevant lines of code and tested it using
> unittests.
> 
> Now what I'm wondering is if it meets the phobos requirements and
> standards.
> And of course if you think I should do a pull request on GitHub!
> 
> My code can be found here, at the bottom of the already existing
> file uri.d:
> https://github.com/MikevanDongen/phobos/blob/uri-parser/std/uri.d

Just some small nit-picks.
* Rename URIerror to URIException
 and make it derive from Exception
 I assume it is allowed to recover from such errors.
* URI_Encode => uriEncode
 in general checkout the Phobos style guide regarding function names
 etc.
 http://dlang.org/dstyle.html
* Why is Uri a class and not a struct? Do you expect people to derive
 from Uri? But then you need to check whether URI.init makes sense.
 According to the Phobos style Uri should be renamed to URI.
* Maybe you should add a constructor in addition to URI.parse()
 to construct some simple URIs.
* Check whether it is const correct. Is const used where it makes sense?
* You could implement opEquals to allow checking for equal URIs.
* Should URIs be only movable? Then you should add
 @disable this(this);
* The code looks sometimes a bit C-ish (gotos) and deeply nested. Maybe
 some functions could be refactored to ease maintenance.
* Regarding documentation. Please state whether it works in CTFE. Probably
 add some unittests.

I hope the module makes it into Phobos. I suggest std.net.uri.
Thank you very much for contributing.

Jens
October 25, 2012
Re: Uri class and parser
On Thursday, October 25, 2012 15:59:39 Jens Mueller wrote:
> * Should URIs be only movable? Then you should add
> @disable this(this);

I'd be shocked if that were appropriate. Disabling the init property like that 
should be done extremely rarely and only if you need to. It's problematic that
that feature exists at all.

> * Regarding documentation. Please state whether it works in CTFE. Probably
> add some unittests.

If it states that it works in CTFE, it needs unit tests to verify it.

> I hope the module makes it into Phobos. I suggest std.net.uri.
> Thank you very much for contributing.

We have std.uri already, so presumably, it would go there.

- Jonathan M Davis
October 25, 2012
Re: Uri class and parser
On Thursday, 25 October 2012 at 13:59:59 UTC, Jens Mueller wrote:
> Just some small nit-picks.

That's what I was hoping for :D


>   in general checkout the Phobos style guide regarding function 
> names
>   etc.
>   http://dlang.org/dstyle.html

>   add some unittests.

Thanks! I haven't read that page before, but I think my code is 
already in that style.
That page referred to one about code coverage, which I didn't 
know is an option in dmd.
I added some unittests and increased the code coverage to 100%.
http://dlang.org/code_coverage.html


> * Rename URIerror to URIException
>   and make it derive from Exception
>   I assume it is allowed to recover from such errors.
> * URI_Encode => uriEncode

I know it was hard to tell, but that's not my code.


> * The code looks sometimes a bit C-ish (gotos) and deeply 
> nested. Maybe
>   some functions could be refactored to ease maintenance.

I'm not sure what functions you mean.


> I hope the module makes it into Phobos. I suggest std.net.uri.

I agree I should move it, so moved my code into a new file, 
'std/net/uri.d'.
https://github.com/MikevanDongen/phobos/blob/uri-parser/std/net/uri.d


> * Why is Uri a class and not a struct? Do you expect people to 
> derive
>   from Uri? But then you need to check whether URI.init makes 
> sense.
>   According to the Phobos style Uri should be renamed to URI.

The renaming is done. At the start, the class was quite general 
in a way I wanted to make others derive from it. That plus the 
little experience I have with structs in D made me use a class.
Now I don't see a reason why anyone would derive from it, so I 
think it should either be a final class (as Jacob Carlborg 
suggested) or a struct.


> * Maybe you should add a constructor in addition to URI.parse()
>   to construct some simple URIs.

Do you mean with parameters (strings?) that simply set the 
properties?


> * Check whether it is const correct. Is const used where it 
> makes sense?

Been thinking about that myself aswell. Both for return values 
and parameters.


> * You could implement opEquals to allow checking for equal URIs.

I like this suggestion! :)


> * Should URIs be only movable? Then you should add
>   @disable this(this);

http://forum.dlang.org/thread/mohceehplxdhsdllxkzt@forum.dlang.org
I'm not sure what movable means or what @disable does. That 
thread doesn't explain it means very clear.


> * Regarding documentation. Please state whether it works in 
> CTFE. Probably

I'll look that up ;)


> I hope the module makes it into Phobos. I suggest std.net.uri.
> Thank you very much for contributing.
>
> Jens

Again, thanks for being supportive; that's what makes me continue 
;)
October 25, 2012
Re: Uri class and parser
On 2012-10-25 15:20, Mike van Dongen wrote:

> It's a dirty job, but someone's gotta do it ;)
> I have commented all code that's not straightforward, but nothing for
> the Ddoc.

If think the ddoc is the most important one.

> Can you give me an example of how specific I need to be in the
> documentation?

I would say as specific as possible. But the interface is pretty 
straight forward. It might be enough to have empty ddoc comments for all 
the methods that is a part of the URI, i.e. "username", "port" and so 
on. Then add documentation for the class that shows how to use the 
complete interface. Take a look the "$.mobile.path.parseUrl()" method 
part of jquerymobile:

http://jquerymobile.com/demos/1.2.0/docs/api/methods.html

It shows an example in the bottom what every "method" would return.


> It's not clear to me what you mean by this.
> To clarify: the first 520 lines weren't written by me, and the code I
> have written doesn't use any of those functions.
> Atleast, for now; Moving the functions 'encode' and 'decode' into the
> class Uri may be useful at a later point.
>
> As I'm the new kid on the block, I'm trying not to break others' code. ;)

Oh, Phobos already has a uri module, then you should leave the other 
code. My bad hehe.

I noticed just now that you moved your new code to std.net.uri. I don't 
think it's good to have two uri modules. Either leave the code in 
std.uri or move all code to std.net.uri and add a public import or 
similar to std.uri. But that is still risk of breaking existing code.

-- 
/Jacob Carlborg
October 25, 2012
Re: Uri class and parser
Jonathan M Davis wrote:
> On Thursday, October 25, 2012 15:59:39 Jens Mueller wrote:
> > * Should URIs be only movable? Then you should add
> > @disable this(this);
> 
> I'd be shocked if that were appropriate. Disabling the init property like that 
> should be done extremely rarely and only if you need to. It's problematic that
> that feature exists at all.

That does not disable the init property. It does disable copying.

> > * Regarding documentation. Please state whether it works in CTFE. Probably
> > add some unittests.
> 
> If it states that it works in CTFE, it needs unit tests to verify it.

Right. Safer this way.

> > I hope the module makes it into Phobos. I suggest std.net.uri.
> > Thank you very much for contributing.
> 
> We have std.uri already, so presumably, it would go there.

I think it belongs inside std.net. Why not make it compatible to the
current std.uri and add deprecated aliases?
The std package is way too flat.

Jens
October 25, 2012
Re: Uri class and parser
Jacob Carlborg wrote:
> On 2012-10-25 15:20, Mike van Dongen wrote:
> 
> >It's a dirty job, but someone's gotta do it ;)
> >I have commented all code that's not straightforward, but nothing for
> >the Ddoc.
> 
> If think the ddoc is the most important one.
> 
> >Can you give me an example of how specific I need to be in the
> >documentation?
> 
> I would say as specific as possible. But the interface is pretty
> straight forward. It might be enough to have empty ddoc comments for
> all the methods that is a part of the URI, i.e. "username", "port"
> and so on. Then add documentation for the class that shows how to
> use the complete interface. Take a look the
> "$.mobile.path.parseUrl()" method part of jquerymobile:
> 
> http://jquerymobile.com/demos/1.2.0/docs/api/methods.html
> 
> It shows an example in the bottom what every "method" would return.
> 
> 
> >It's not clear to me what you mean by this.
> >To clarify: the first 520 lines weren't written by me, and the code I
> >have written doesn't use any of those functions.
> >Atleast, for now; Moving the functions 'encode' and 'decode' into the
> >class Uri may be useful at a later point.
> >
> >As I'm the new kid on the block, I'm trying not to break others' code. ;)
> 
> Oh, Phobos already has a uri module, then you should leave the other
> code. My bad hehe.
> 
> I noticed just now that you moved your new code to std.net.uri. I
> don't think it's good to have two uri modules. Either leave the code
> in std.uri or move all code to std.net.uri and add a public import
> or similar to std.uri. But that is still risk of breaking existing
> code.

I'd prefer the second option. Maybe write first some unittests for
std.uri, if there are none. Then move it.

Jens
October 25, 2012
Re: Uri class and parser
Mike van Dongen wrote:
> On Thursday, 25 October 2012 at 13:59:59 UTC, Jens Mueller wrote:
> >Just some small nit-picks.
> 
> That's what I was hoping for :D
> 
> 
> >  in general checkout the Phobos style guide regarding function
> >names
> >  etc.
> >  http://dlang.org/dstyle.html
> 
> >  add some unittests.
> 
> Thanks! I haven't read that page before, but I think my code is
> already in that style.
> That page referred to one about code coverage, which I didn't know
> is an option in dmd.
> I added some unittests and increased the code coverage to 100%.
> http://dlang.org/code_coverage.html

Are you sure? I've seen _ in names where camel case should have been used.

> >* Rename URIerror to URIException
> >  and make it derive from Exception
> >  I assume it is allowed to recover from such errors.
> >* URI_Encode => uriEncode
> 
> I know it was hard to tell, but that's not my code.

You can still change it, can't you?
Just want to see the code being improved.

> >* The code looks sometimes a bit C-ish (gotos) and deeply nested.
> >Maybe
> >  some functions could be refactored to ease maintenance.
> 
> I'm not sure what functions you mean.

There are functions that I find difficult to understand. Usually the
ones with lots of deeply nested control statements. And often they are a
bit long. I will read it later again and be more specific.

> >I hope the module makes it into Phobos. I suggest std.net.uri.
> 
> I agree I should move it, so moved my code into a new file,
> 'std/net/uri.d'.
> https://github.com/MikevanDongen/phobos/blob/uri-parser/std/net/uri.d

Now we need to properly deprecate the old std.uri without breaking any
code. Do you think this is feasible?
And are the Phobos core developers okay with this?

> >* Why is Uri a class and not a struct? Do you expect people to
> >derive
> >  from Uri? But then you need to check whether URI.init makes
> >sense.
> >  According to the Phobos style Uri should be renamed to URI.
> 
> The renaming is done. At the start, the class was quite general in a
> way I wanted to make others derive from it. That plus the little
> experience I have with structs in D made me use a class.
> Now I don't see a reason why anyone would derive from it, so I think
> it should either be a final class (as Jacob Carlborg suggested) or a
> struct.

Do you want reference semantics or value semantics? I find value
semantics easier. So I would go with a struct. When passing URIs around
one can still pass by reference.

> >* Maybe you should add a constructor in addition to URI.parse()
> >  to construct some simple URIs.
> 
> Do you mean with parameters (strings?) that simply set the
> properties?

Yes.
At least to support constructing often used URIs.
And then maybe another constructor that calls parse for all other cases.

> >* Check whether it is const correct. Is const used where it makes
> >sense?
> 
> Been thinking about that myself aswell. Both for return values and
> parameters.

It's annoying to do it later but it should be done, I think. And it's
not too late. Good luck!

> >* You could implement opEquals to allow checking for equal URIs.
> 
> I like this suggestion! :)
> 
> 
> >* Should URIs be only movable? Then you should add
> >  @disable this(this);
> 
> http://forum.dlang.org/thread/mohceehplxdhsdllxkzt@forum.dlang.org
> I'm not sure what movable means or what @disable does. That thread
> doesn't explain it means very clear.

In my understanding structs are moved by default. That means the members
a bit-wise copied. Now assuming you move a pointer, then you may want to
copy the contents that the pointer to have an independent object, a copy
so to say.
This is achieved by using the so called postblit constructor this(this).
If you disable it, then the URIs are only movable. If you want copying,
then you may need to implement it.
http://dlang.org/struct.html#StructPostblit

> >* Regarding documentation. Please state whether it works in CTFE.
> >Probably
> 
> I'll look that up ;)
> 
> 
> >I hope the module makes it into Phobos. I suggest std.net.uri.
> >Thank you very much for contributing.
> >
> >Jens
> 
> Again, thanks for being supportive; that's what makes me continue ;)

I hope there will be a happy end.
We should probably seek some advice from the core Phobos developers
whether they want to have an improved std.uri moved to std.net.uri.
Don't want you to put work into something that is unlikely to make
through the review process.

Jens
October 25, 2012
Re: Uri class and parser
On Thursday, October 25, 2012 23:05:14 Jens Mueller wrote:
> Jonathan M Davis wrote:
> > On Thursday, October 25, 2012 15:59:39 Jens Mueller wrote:
> > > * Should URIs be only movable? Then you should add
> > > @disable this(this);
> > 
> > I'd be shocked if that were appropriate. Disabling the init property like
> > that should be done extremely rarely and only if you need to. It's
> > problematic that that feature exists at all.
> 
> That does not disable the init property. It does disable copying.

Ah true. I read it too quickly. Regardless, disabling much of _anything_ is 
something that you shouldn't be doing normally (though disabling init is 
definitely the worst). It's far, far better to implement the postblit when a 
shallow copy doesn't do what you want. 

- Jonathan M Davis
1 2 3 4 5 6
Top | Discussion index | About this forum | D home