raku-community-modules / URI

Raku realization of URI — Uniform Resource Identifiers handler
Artistic License 2.0
3 stars 14 forks source link

Adding mutators and major code cleanup/documentation/test expansion #34

Closed zostay closed 6 years ago

zostay commented 7 years ago

This is a fairly major patch that adds mutators to URI (see issue #27) and also fixes how the query-form is returns (see issue #33).

It adds new class and types for maintaining the correctness of the URI while modified and to provide piecewise encapsulation of data. This includes adding a number of subsets for validating the components and the URI::Query, URI::Authority, and URI::Path classes for maintaining state specific to those components of the URI.

The most controversial decision I think I made is a preference for what I call "heavy mutators" which are defined as methods on the class rather than as assignment. For example,

my $u = URI.new("mailto:bob@example.com");
$u.path("steve@example.com");

This is due to the fact that most of the mutators have to do some amount of specialized validation, conditional object construction, and cross-checking with other fields in order to keep the state of URI pure. The only way to achieve this with assignment is to use a lot of Proxy objects, but I do not believe this to be a good practice. (Historically, I have done just this on other work I've done and I believe that has often been a mistake.)

The other controversial decision I made is to deprecate query-form and make hash-like use of query data always work with lists of strings rather than a mixed form (see #33). However, because of how lists work in Perl 6, I believe there's a very good chance most existing uses of query-form will work correctly despite this. If not, there is a hash-format option for quickly recovering the old behavior. I will also say that I'd be willing to modify the code so that the deprecated query-form method always returns the mixed form and recommend everyone update their code to use query in the future.

I have expanded the test suite quite a bit and have attempted to thoroughly document everything URI is capable of doing going forward.

ronaldxs commented 7 years ago

It appears to me that the README.md was not updated ...

ronaldxs commented 7 years ago

Please add: Ilya Belikin(ihrd) To the list of contributors. He wrote a regex version that comprised the first four commits of the repository. Thanks.

ronaldxs commented 7 years ago

I tried to run some of the examples from the README and a few did not work.

For example setting the userinfo:

ron@ws-ubuntu:~/perl6/zostay-uri$ perl6 -Ilib -MURI -e 'my $u = URI.new(q!http://example.com/foo/bar?tag=woow#bla!); .userinfo("bob") with $u.authority'
Too many positionals passed; expected 1 argument but got 2
  in method userinfo at /home/ron/perl6/zostay-uri/lib/.precomp/7A5C2A15488B7F2C467D33F04E36BD2EE0BCA9C1.1499026043.26079/CB/CB371C93C5AA0E62198EFD303AE2C17474416D1A line 1
  in block <unit> at -e line 1

The current userinfo setter method signatures all specify a URI object as invocant not an Authority. My best guess is that there should be a setter methods with both invocants, with the userinfo setter for URI just calling something like .authority.userinfo($new_userinfo) for its authority property.

In updating the query later, the pair lookup and list index seem backwards:

ron@ws-ubuntu:~/perl6/zostay-uri$ perl6 -Ilib -MURI -e 'my $u = URI.new(q!http://example.com/foo/bar?tag=woow#bla!); with $u.query {.query("foo=1&foo=2"); say .query-form<foo>[0] }'
Type Array[Pair] does not support associative indexing.
  in block <unit> at -e line 1

ron@ws-ubuntu:~/perl6/zostay-uri$ perl6 -Ilib -MURI -e 'my $u = URI.new(q!http://example.com/foo/bar?tag=woow#bla!); with $u.query {.query("foo=1&foo=2"); say .query-form[0]<foo> }'
1

After fixing the indexing above, the example of .query-form.push: 'bar' => 'ok'; doesn't seem to change the topic object. You might want to go through and retest your documentation examples. I will try to retest if later commits make relevant changes.

Thanks

jonathanstowe commented 6 years ago

Hi, I've moved this into https://github.com/perl6-community-modules/uri/tree/zostay-mutators so we can move it on a bit. All looks good.