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

Remove `UnderscoreSupportingURI`. #3259

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
4 participants
@reitermarkus
Member

reitermarkus commented Oct 3, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Not needed anymore on Ruby 2.3.3.

@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Oct 3, 2017

Member

I’m a bit on the fence here. While it’s true that the issue has been fixed since Ruby 2.2, and it makes perfect sense to ditch UnderscoreSupportingURI, I’m not sure if we must really lose the spec.

After all, we rely on the new behavior, and the spec should reflect this. Basically, it would act as a canary, which would give alarm in case HBC inadvertently fails to switch to the vendored Ruby 2.3.3, or falls back to Ruby 2.0 for some other reason.

On the other hand, testing an upstream API should be none of our business.

Would it hurt to keep the spec, and have it point to URI? I’m torn tbh.

Member

claui commented Oct 3, 2017

I’m a bit on the fence here. While it’s true that the issue has been fixed since Ruby 2.2, and it makes perfect sense to ditch UnderscoreSupportingURI, I’m not sure if we must really lose the spec.

After all, we rely on the new behavior, and the spec should reflect this. Basically, it would act as a canary, which would give alarm in case HBC inadvertently fails to switch to the vendored Ruby 2.3.3, or falls back to Ruby 2.0 for some other reason.

On the other hand, testing an upstream API should be none of our business.

Would it hurt to keep the spec, and have it point to URI? I’m torn tbh.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 3, 2017

Member

Basically, it would act as a canary, which would give alarm in case HBC inadvertently fails to switch to the vendored Ruby 2.3.3, or falls back to Ruby 2.0 for some other reason.

A bunch of other things will fail before that point. I'm 👍 to deleting this if it's an upstream API. Keeping stuff like it around bloats the test suite, a bit.

Member

MikeMcQuaid commented Oct 3, 2017

Basically, it would act as a canary, which would give alarm in case HBC inadvertently fails to switch to the vendored Ruby 2.3.3, or falls back to Ruby 2.0 for some other reason.

A bunch of other things will fail before that point. I'm 👍 to deleting this if it's an upstream API. Keeping stuff like it around bloats the test suite, a bit.

@claui

This comment has been minimized.

Show comment
Hide comment
@claui

claui Oct 3, 2017

Member

A bunch of other things will fail before that point.

That’s a good point, and comforting to know.
In that case, LGTM as is, too.

Member

claui commented Oct 3, 2017

A bunch of other things will fail before that point.

That’s a good point, and comforting to know.
In that case, LGTM as is, too.

@reitermarkus reitermarkus merged commit 0fbcbc7 into Homebrew:master Oct 4, 2017

3 checks passed

codecov/patch 100% of diff hit (target 67.14%)
Details
codecov/project 67.14% (+<.01%) compared to ec0d8fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reitermarkus reitermarkus deleted the reitermarkus:underscore_supporting_uri branch Oct 4, 2017

@rednoah

This comment has been minimized.

Show comment
Hide comment
@rednoah

rednoah Nov 16, 2017

Contributor

signature may be :embedded for GPG container files that have embedded signatures. URI() raises an exception for :embedded so behaviour is slightly different to UnderscoreSupportingURI.parse().

Here's the related test case from history which has been removed during cleanup at some point:
5c59b33#diff-400867ba2024572d1c057e13354d61b6

Contributor

rednoah commented on Library/Homebrew/cask/lib/hbc/dsl/gpg.rb in a92b631 Nov 16, 2017

signature may be :embedded for GPG container files that have embedded signatures. URI() raises an exception for :embedded so behaviour is slightly different to UnderscoreSupportingURI.parse().

Here's the related test case from history which has been removed during cleanup at some point:
5c59b33#diff-400867ba2024572d1c057e13354d61b6

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

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