-
Notifications
You must be signed in to change notification settings - Fork 351
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
[Source] Remove the notion of a DataProvider
and move logic into Source
#184
Conversation
Let me just test that the CocoaPods specs still pass with these changes. |
Specs pass with CocoaPods/CocoaPods#2600 |
else | ||
@data_provider = repo | ||
end | ||
@repo = Pathname.new(repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw if repo
is nil
and since that is the default value of repo
it seems likely to happen at some point. Is there any reason for repo
to still default to nil
and if there is shouldn't the resulting nil
value be handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
@segiddins I wonder why it even has a default value, is it to make the tests simpler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check through Core
and CocoaPods
and I saw no usage of Source.new()
or Source.new(nil)
. I guess maybe it could be called in a way that doesn't match this search e.g when stubbing or similar
Once that default value has been resolved than this should be good to be merged 👍 |
[Source] Remove the notion of a `DataProvider` and move logic into `Source`
👍 |
[Source] Remove the notion of a `DataProvider` and move logic into `Source`
Closes #183.
Closes #182.
\c @fabiopelosin @alloy