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

Give Sources and SourceProviders a url attribute #174

Merged
merged 7 commits into from
Sep 25, 2014
Merged

Conversation

segiddins
Copy link
Member

This is needed for CocoaPods/CocoaPods#2513.

@@ -64,6 +64,9 @@ DotPosition:
EachWithObject:
Enabled: false

Style/SpecialGlobalVars:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use # rubocop:disable Style/SpecialGlobalVars for the relevant line instead. This would allow the check to stay in place or the rest of the code which is preferable.

Copy link
Member Author

Choose a reason for hiding this comment

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

That decision is left up to @alloy and @fabiopelosin

Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit like an overreach to disable it globally for one infraction that's my only concern with it

Copy link
Member

Choose a reason for hiding this comment

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

As long as we support Ruby 1.8.7 we are completely disabling it (because adding a comment to each line with $? would be inconvenient and style checks should not be inconvenient).

Copy link
Member

Choose a reason for hiding this comment

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

I don’t know if this config entry is about anything other than the use of the ‘English’ lib. If it’s only about that, then we can disable it globally, otherwise disabling it for just that line makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

As long as we support Ruby 1.8.7

@fabiopelosin Interesting. Are you saying that Ruby 2 has the English variants by default?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, they’re simply not available at all on 1.8. Gotcha 👍

@fabiopelosin
Copy link
Member

The GHDataProvider is not being currently used and can be removed. I implemented it because at the time I was of the opinion (and to a certain extent I still do) that using git to download the master repo will lead to performance issues as the repo grows in size.

However supporting multiple strategies for the sources has drawbacks, one among them is that a strategy based on the GitHub APIs would not be easily adaptable to the awesome evaluations that lead to this patch. Being life a matter of choosing among trade-offs, now I think that the GHDataProvider
should be removed.

@alloy
Copy link
Member

alloy commented Sep 25, 2014

Being life a matter of choosing among trade-offs, now I think that the GHDataProvider
should be removed.

❤️ 👍

(And it’s still in the commit history when we do decide otherwise.)

@segiddins
Copy link
Member Author

We can always add it back if we change our minds.

-Samuel E. Giddins

On Sep 25, 2014, at 3:47 AM, Eloy Durán notifications@github.com wrote:

Being life a matter of choosing among trade-offs, now I think that the GHDataProvider
should be removed.


Reply to this email directly or view it on GitHub.=

segiddins added a commit that referenced this pull request Sep 25, 2014
Give Sources and SourceProviders a url attribute
@segiddins segiddins merged commit b9dcb24 into master Sep 25, 2014
@segiddins segiddins deleted the source-url branch September 25, 2014 15:57
Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
Give Sources and SourceProviders a url attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants