-
Notifications
You must be signed in to change notification settings - Fork 48
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
Escape square brackets in path #100
Conversation
Hopefully fixes issue#99 w/o breaking new stuff. libwww-perl#99 Setting the environment variable URI_RESERVED_SQUARE_BRACKETS=1 restores the old behaviour (5.10 and before). See section ENVIRONMENT VARIABLES of the URI.pm perldoc.
Not complete yet. See #TODO.
Hi Olaf (@oalders), I've fixed the typos that prevented the workflow from succeeding and fixed some setters while adding more tests. Perhaps a note to CONTRIBUTING.md could be added that a successful installation of Thanks |
Fixed POD again m( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this PR. This is really good work.
I've left a few comments for discussion, and there is a POD issue that needs addressing before our tests can pass.
As for your documentation, from one non-native speaker to another, you don't have to worry about the quality of your English. It's great, and sounds very professional.
It looks like my review tab didn't refresh when you pushed the POD fix. I've amended that now. |
The fallback regexp is just a check for the minimum amount of legal characters. Extra check is used to ensure that at least two colons are present. See: libwww-perl#100 (comment)
Hi Olaf (@oalders), I consider the work ready for the next iteration (of spell checking). |
@simbabque looks like this is ready for another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes. You asked for spell checking, so I found a few typos.
Joking aside, this is very good. I particularly like the extensive test cases. Please add Regexp::IPv6 as a suggested or recommended dependency. That way users don't have to install it, but can know that it's useful to have. We should consider that this module hasn't been touched in over 10 years and has open issues though.
Hi Olaf (@oalders), seems, it's ready for another (final?) workflow approval. Thanks. |
Build is now queued. 😄 |
@simbabque all checks are now passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, it looks good to me now!
bugfix #99: Square brackets in path element not escaped. Hopefully fixes issue#99 w/o breaking new stuff. #99 Setting the environment variable URI_RESERVED_SQUARE_BRACKETS=1 restores the old behaviour (5.10 and before). See section ENVIRONMENT VARIABLES of the URI.pm perldoc. Tests for issue#99: Square brackets in path element not escaped. Not complete yet. See #TODO. t/old-base.t adapted to match new and legacy behavior Fixed typos in POD of URI.pm more tests, esp. getter validation userinfo(): setter escapes according to RFC 3987 authority(): setter escapes userinfo part separately from host part authority(): IPv6 detection for host part (should use Regexp::IPv6) URI.pm: POD fixed uri-test: explicit use of ./lib replaced by visual warning See: #100 (comment) POD corrections and clarifications. See: #100 (comment) #100 (comment) #100 (comment) Unused global variable removed. See: #100 (comment) _generic.pm: uses Regexp::IPv6 if installed, fallback otherwise The fallback regexp is just a check for the minimum amount of legal characters. Extra check is used to ensure that at least two colons are present. See: #100 (comment) Another try to get POD spehlink rait. cpanfile: Regexp::IPv6 is suggested at runtime Spelling fixed and comments updated. Thanks, @simbabque.
Thanks @Perlbotics and @simbabque! I squashed the commits at the command line and merged them into |
I've uploaded URI 5.11 to CPAN. It was a long journey. Thanks for being patient. 😄 |
Hi Olaf (@oalders) - here's the pull request.
I've split the commits into groups for easy cherrypicking.
Cheers/PB
Fixes #99