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

Move depracted? method from Presenter to a DSL helper in specification #157

Closed
wants to merge 1 commit into from

Conversation

k0nserv
Copy link
Member

@k0nserv k0nserv commented Jul 29, 2014

I realised that it doesn't make much sense to have this logic in the presenter when I was looking at the implementation of https://github.com/CocoaPods/search.cocoapods.org. If you agree with this move I'll update the change I made to https://github.com/CocoaPods/CocoaPods

@fabiopelosin
Copy link
Member

It should be moved to https://github.com/CocoaPods/Core/blob/master/lib/cocoapods-core/specification/root_attribute_accessors.rb (because the spec class is too bloated and it is a root attribute).

@k0nserv
Copy link
Member Author

k0nserv commented Jul 30, 2014

Okay. I put it as a DSL helper because in reality it's a combination of two root attributes, from reading the root attribute code it looked like it was just doing mapping without any logic. I also think there could be a problem with having both deprecated and deprecated? because they are not distinct. is_depracted? is slightly better for the name, but still a bit undescriptive

@k0nserv
Copy link
Member Author

k0nserv commented Aug 9, 2014

Ping @irrationalfab any comments on my last comment?

@fabiopelosin
Copy link
Member

from reading the root attribute code it looked like it was just doing mapping without any logic.

There is some logic in some attributes: https://github.com/CocoaPods/Core/blob/master/lib/cocoapods-core/specification/root_attribute_accessors.rb#L49. Although you are correct to say that the all methods mirror the attributes. However it is more important to group root attributes there than to adhere to the above observation.

I also think there could be a problem with having both deprecated and deprecated? because they are not distinct. is_depracted? is slightly better for the name, but still a bit undescriptive

is_depracted? is not a candidate, because in Ruby the final question mark means is_.

What I would do is: move the stuff to the root attribute accessor because all the root attributes are grouped there (this is not the state of the art of software architecture but until is refactored this is the logic). There should be two methods there deprecated? (returns if the Pod is deprecated, regardless of how the deprecation was specified which is not interesting to clients) and deprecated_in_favor_of (which returns nil if there is not an alternative Pod).

This is my take.

… from Presenter to root attributes in specification
@fabiopelosin
Copy link
Member

I have rebased and merged this patch via 1be2e4e. Thanks @k0nserv!

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.

2 participants