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

Add support for `fragment` in `parse_url` and `build_url` #70

Merged
merged 1 commit into from Feb 12, 2014

Conversation

Projects
None yet
2 participants
@craigcitro
Contributor

craigcitro commented Feb 8, 2014

Any opposition to adding support for the fragment? It looks like parse_url already grabs it.

@hadley

This comment has been minimized.

Member

hadley commented Feb 8, 2014

Probably just an oversight

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Feb 8, 2014

(Amended my commit when I noticed something trivial; on the off chance that bit me just lemme know and I'll fix.)

@hadley

This comment has been minimized.

Member

hadley commented Feb 10, 2014

Commit looks fine but I'll show you a nicer idiom when I'm at my computer

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Feb 10, 2014

+1 -- this means my not-so-secret plan to learn R by having you review my code is working. ;)

@hadley

This comment has been minimized.

Member

hadley commented Feb 10, 2014

I'd write it like:

str_c(scheme, "://", login, hostname, port, "/", path, params, query, 
    if (!is.null(url$fragment)) "#", fragment)

str_c() automatically drops zero-length inputs, which is a really useful convention because it an if without an else block returns NULL for FALSE.

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Feb 11, 2014

ah, nice -- done.

if you want, i can do the same to the username/password lines above, since that would all compact down nicely. (and probably add a stop for the case of password without username?)

@hadley

This comment has been minimized.

Member

hadley commented Feb 11, 2014

Yes please!

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Feb 12, 2014

done -- PTAL

@hadley

This comment has been minimized.

Member

hadley commented Feb 12, 2014

Looks good. Can you please add a bullet to the NEWS?

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Feb 12, 2014

ok, added a note and squashed.

hadley added a commit that referenced this pull request Feb 12, 2014

Merge pull request #70 from craigcitro/fragment
Add support for `fragment` in `parse_url` and `build_url`

@hadley hadley merged commit 018ec10 into r-lib:master Feb 12, 2014

1 check passed

default The Travis CI build passed
Details

@craigcitro craigcitro deleted the craigcitro:fragment branch Mar 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment