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

Escape square brackets in path #100

Closed
wants to merge 17 commits into from
Closed

Conversation

Perlbotics
Copy link

@Perlbotics Perlbotics commented Jun 7, 2022

Hi Olaf (@oalders) - here's the pull request.
I've split the commits into groups for easy cherrypicking.
Cheers/PB

Fixes #99

@Perlbotics
Copy link
Author

Hi Olaf (@oalders),

I've fixed the typos that prevented the workflow from succeeding and fixed some setters while adding more tests.
I could also run dzil w/o problems (on URI package only).

Perhaps a note to CONTRIBUTING.md could be added that a successful installation of
spellchecking modules requires a LANG=C environment?
At least Test::Spelling and perhaps also Dist::Zilla::Plugin::Test::PodSpelling required this setting here on my host.
I.e. LANG=C dzil authordeps --missing | cpanm

Thanks
PB

@oalders oalders changed the title Bug99 Don't escape square brackets in path Jun 9, 2022
@oalders oalders changed the title Don't escape square brackets in path Escape square brackets in path Jun 9, 2022
@oalders oalders requested a review from FGasper June 9, 2022 10:21
@Perlbotics
Copy link
Author

Fixed POD again m(
Sorry, not catched before.

Copy link
Contributor

@simbabque simbabque left a 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.

lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
lib/URI/_generic.pm Outdated Show resolved Hide resolved
uri-test Outdated Show resolved Hide resolved
@simbabque
Copy link
Contributor

It looks like my review tab didn't refresh when you pushed the POD fix. I've amended that now.

lib/URI.pm Outdated Show resolved Hide resolved
@oalders oalders removed the request for review from FGasper June 22, 2022 15:39
@Perlbotics
Copy link
Author

Hi Olaf (@oalders), I consider the work ready for the next iteration (of spell checking).
Please approve the build workflow to see what happens next. Thank you.

@oalders
Copy link
Member

oalders commented Jun 29, 2022

@simbabque looks like this is ready for another review.

Copy link
Contributor

@simbabque simbabque left a 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.

lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
lib/URI/_generic.pm Outdated Show resolved Hide resolved
lib/URI/_generic.pm Outdated Show resolved Hide resolved
@Perlbotics
Copy link
Author

Hi Olaf (@oalders), seems, it's ready for another (final?) workflow approval. Thanks.

@oalders
Copy link
Member

oalders commented Jul 4, 2022

Build is now queued. 😄

@oalders
Copy link
Member

oalders commented Jul 4, 2022

@simbabque all checks are now passing.

Copy link
Contributor

@simbabque simbabque left a 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!

oalders pushed a commit that referenced this pull request Jul 4, 2022
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.
@oalders
Copy link
Member

oalders commented Jul 4, 2022

Thanks @Perlbotics and @simbabque! I squashed the commits at the command line and merged them into master via f418311.

@oalders
Copy link
Member

oalders commented Jul 4, 2022

I've uploaded URI 5.11 to CPAN. It was a long journey. Thanks for being patient. 😄

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

Successfully merging this pull request may close these issues.

Square brackets in path element not escaped.
4 participants