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

[fix] Escape ref in archive_link #1375

Merged
merged 5 commits into from
May 24, 2022

Conversation

max611
Copy link
Contributor

@max611 max611 commented Oct 20, 2021

When using archive_link with Unicode characters we have the following error: URI must be ascii only "repos/octokit/octokit.rb/tarball/\u{1F419}\u{1F431}"

This PR fixes the issue with using CGI.escape but I am open to any other alternative, I saw that CGI.escape was already used in the codebase.

This is a breaking change since it's possible that someone was already using CGI.escape and we would escape it twice.

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @max611 thank you for putting in the work to make this change ❤️ . I left a comment about using ERB vs CGI - let me know what you think about the suggestion and we'll try to get this merged in. Thanks!

lib/octokit/client/contents.rb Outdated Show resolved Hide resolved
Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
@nickfloyd nickfloyd added the bug label May 24, 2022
@nickfloyd nickfloyd merged commit ce8e5ce into octokit:4-stable May 24, 2022
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed bug labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants