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 question marks in URLs #558

Closed
wants to merge 1 commit into from

Conversation

PaulTaykalo
Copy link

Source Kitten output could have question marks in document names when parsing Swift sources. Since we cannot use question marks in URLs, we need to escape them with some adequate characters sequence. q was added to represent question mark, and - was added since no potential function name can have dashes in it

@PaulTaykalo
Copy link
Author

This one is the fix for #547

@jpsim
Copy link
Collaborator

jpsim commented May 16, 2016

Is there a way we could apply a more general URL sanitization step?

@PaulTaykalo
Copy link
Author

@jpsim for example? Are there any other issues related to the URLs sanitization?

@jpsim
Copy link
Collaborator

jpsim commented May 16, 2016

Yes, there have been issues with Windows file names before too, which is why I'd like to implement a general solution to URL escaping rather than hardcoding this case, without addressing possible others.

@PaulTaykalo
Copy link
Author

PaulTaykalo commented May 16, 2016

@jpsim like this one #361?
It's not technically escaping. Well, its' not URL escaping, As far as I see - valid file name without special characters should be generated. As well as valid URL. Haven't seen any library/code that will do that conversion for all OSes.
So currently there should be only these \ / : * ? " < > |
I would suggest to handle it in the same way, unless there's better option
What do you think?

So I suggest by simply adding handlign for these characters, and handle them in tests in https://github.com/realm/jazzy-integration-specs/tree/master/misc_jazzy_features

@jpsim
Copy link
Collaborator

jpsim commented May 16, 2016

That seems fine to me!

@pigeon-archive
Copy link
Contributor

Since the original solution proposed in this PR was rejected in favour of a more generic one, I'm going to close this PR. Feel free to re-open or file a new PR with the new/preferred solution.

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

Successfully merging this pull request may close these issues.

3 participants