remove pipe characters from URLs #46

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@wopr42

I have a project that uses to_url on page titles, which sometimes have pipes. URI.parse barfed. It's a legal character but should be encoded, so I figured this is the place to make the change.

@rsl
Owner

did some reading and this seems to cause some issues in other areas too so seems like a good call. i'm just not sure that removing it is the right replacement. not sure it's not either. just thinking about possible word transcriptions like happen with $ and / => dollar and slash. thinking "pipe" makes the most sense but not sure how many usages of this wouldn't fit? what's yr thoughts?

@wopr42

I've only seen it used as a separator between the site name and page title. Replacing it with a literal "pipe" should be fine, just don't think it adds any value to the cases I've seen. The authors aren't trying to say "pipe." That said, I could see cases where they would be, and it's not going to make a big difference for me either way.

@rsl
Owner

yeah i have no idea. will sleep on this and do something in the morning.

@dblock

I vote +1. We use to_url in mongoid-slug and basically want all special characters removed.

@stormsilver

+1 for this. Just fixed it myself, then thought to check to see if there was a pull request fixing the problem. :/ I agree that simply removing the pipe is the right choice. Most of the time when I've seen a pipe it's used as a separator, not to provide semantic meaning.

@stormsilver stormsilver added a commit to stormsilver/stringex that referenced this pull request Jun 26, 2012
@stormsilver stormsilver Added pipe striping code from upstream pull req #46, moved dup slug c…
…hecking code so that it doesn't run unless necessary.
ef6cf13
@rsl
Owner

yeah i have no strong feelings about transliterating that as 'pipe' so this seems fine.

@rsl
Owner

feeling like idiot but i can't find the commit to pull in here in yr repo

@rsl
Owner

manually added this in 4fdc79d. thanks for the work on this. sorry i was so slack in taking care of it. @stormsilver, i'd already merged in an equivalent for yr dup slug optimization. good job though.

@rsl rsl closed this Nov 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment