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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default subspecs #128

Merged
merged 2 commits into from May 19, 2014
Merged

Default subspecs #128

merged 2 commits into from May 19, 2014

Conversation

kylef
Copy link
Contributor

@kylef kylef commented May 2, 2014

馃嵒

#
# The name of the subspec that should be used as preferred dependency.
# An array of subspecs names that should be used as preferred dependency.

Choose a reason for hiding this comment

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

Line is too long. [81/80]

@fabiopelosin
Copy link
Member

The singular form should continue working in a similar fashion to the other DSL attributes.

@fabiopelosin
Copy link
Member

I.e. just drop the deprecation. Ace work btw!

@kylef
Copy link
Contributor Author

kylef commented May 3, 2014

@irrationalfab What should happen if you set both default_subspecs and default_subspec? Simply dropping 11445bc should be fine yes?

@fabiopelosin
Copy link
Member

The behaviour of the rest of the attributes when they are redeclared (a misuse of the DSL) is to replace the value.

The patch looks really good, but it should be coherent with the rest of the attributes logic:

@fabiopelosin
Copy link
Member

Yeah almost all of the 11445bc should be dropped. Note that Specification::Consumer takes care of boxing the raw value of the in the container if needed.

I don't recall the reason why this is being done during the read instead of during the writing (which if I recall correctly used to be the case in the past). I need to recheck and change if appropriate, this might be relevant to the JSON conversion.

/c @alloy

@kylef
Copy link
Contributor Author

kylef commented May 4, 2014

Thanks for the pointers @irrationalfab.

I'm trying to implement this now, however the boxing mechanism isn't working as I expected. My attribute looks like the following:

attribute :default_subspecs, :container => Array, :singularize => true, :multi_platform => false

Now, spec.default_subspec = 'Pod' is resulting in spec.default_subspecs to be equal to Pod and not an array containing Pod. Am I doing this correctly?

@fabiopelosin
Copy link
Member

Now, spec.default_subspec = 'Pod' is resulting in spec.default_subspecs to be equal to Pod and not an array containing Pod. Am I doing this correctly?

I don't recall if there is an exact reason for this (I refactored the Specification class so many times, consider it a strange hobby), but to get the boxing and all the rest of the logic the Specification::Consumer should be used (which at the moment is lacking the support for the default subspec, maybe because it was not needed and because it is not multi-platform).

So to get the boxing something like specification.consumer(platform).default_subspecs should be used. Subspecs like all the other attributes make practical sense only in the context of a platform. Although this might look inconvenient for the time being I would use it and consider a refactor after.

@@ -377,7 +377,7 @@ module Pod
spec.should.respond_to(attr.writer_name)
end
singularized.map { |attr| attr.name.to_s }.sort.should == %w(
authors compiler_flags frameworks libraries preserve_paths
authors compiler_flags default_subspecs frameworks libraries preserve_paths

Choose a reason for hiding this comment

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

Line is too long. [85/80]

@kylef
Copy link
Contributor Author

kylef commented May 4, 2014

Alright thanks for going over that. I noticed that there were no tests for the other singularised attributes in the way I was doing them. So i've removed these tests and updated the pull request.

Take a look and tell me if there's anything else to add 馃憤.

@kylef
Copy link
Contributor Author

kylef commented May 16, 2014

Thanks for the support on this one @irrationalfab 馃憤.

@fabiopelosin
Copy link
Member

Ace!

fabiopelosin added a commit that referenced this pull request May 19, 2014
@fabiopelosin fabiopelosin merged commit cdcfd10 into master May 19, 2014
@fabiopelosin fabiopelosin deleted the default_subspecs branch May 19, 2014 09:50
Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
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.

None yet

3 participants