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: Addresses URI parsing compatibility with Ruby 3.4 #1708

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

casperisfine
Copy link

But also older Rubies if the default URI parser is set to RFC3986.

Copy link

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

But also older Rubies if the default URI parser is set
to `RFC3986`.
@nickfloyd
Copy link
Contributor

Hey @casperisfine, this change set looks good. Thanks for doing it! ❤️

Would you mind adding some tests? If not I can do that later. I'll approve either way and thanks again!

Something like:

describe '#validate_owner_and_name!' do
    let(:validator) { described_class.new(owner, name, url) }
    let(:repo) { 'octokit_repo' }

    context 'when owner and name are valid and url is absolute' do
      let(:owner) { 'valid_owner' }
      let(:name) { 'valid_name' }
      let(:url) { 'http://octokit.com' }

      it 'does not raise an error' do
        expect { validator.validate_owner_and_name!(repo) }.not_to raise_error
      end
    end

    context 'when owner contains a slash' do
      let(:owner) { 'invalid/owner' }
      let(:name) { 'valid_name' }
      let(:url) { 'http://octokit.com' }

      it 'raises an invalid repository error' do
        expect { validator.validate_owner_and_name!(repo) }.to raise_error(InvalidRepositoryError)
      end
    end

@casperisfine
Copy link
Author

It's already covered by tests in spec/octokit/repository_spec.rb.

The issue is that the Gemfile.lock contains uri (0.13.0) so even on Ruby head it's not yes using the broken version of URI.

The test will only fail once these changes are released and updated in the Gemfile: ruby/uri#107

@nickfloyd nickfloyd changed the title Fix compatibility with Ruby 3.4 fix: Addresses URI parsing compatibility with Ruby 3.4 Jul 24, 2024
@nickfloyd nickfloyd added the Type: Bug Something isn't working as documented label Jul 24, 2024
@nickfloyd nickfloyd merged commit 7ff23fa into octokit:main Jul 24, 2024
14 checks passed
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants