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

Add ObjectReference #218

Merged
merged 3 commits into from
Jan 23, 2018
Merged

Add ObjectReference #218

merged 3 commits into from
Jan 23, 2018

Conversation

yonaskolb
Copy link
Collaborator

@yonaskolb yonaskolb commented Jan 22, 2018

Now that PBXObject no longer contains a reference, the helper methods that return a PBXObject are hard to work with as you don't know the reference, for example targets(named: String) -> PBXTarget

This can also be evidenced by the new helper functions in #213 that all return a tuple of the reference and the object.

This PR adds a new genenric type ObjectReference that contains the reference and the object. This is also used in XcodeGen to ease the migration to the removal of reference from PBXObject

This is actually a breaking change. I was hoping to get this into 3.0 but I didn't know it was releasing early.

Another addition to the Objects class that could have also helped is the addition of a getReference(object: PBXObject) -> String that would return the reference for a given object using equatable or class identity, but I thought this would be a cleaner approach

This PR also makes PBXObject Equatable


This change is Reviewable

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Great idea @yonaskolb. Don't forget to generate the Carthage project and update the changelog. It looks good to me. I plan to add the Sourcery feature this week to include it in the release 4.0, that will also include this change. What do you think?

@yonaskolb
Copy link
Collaborator Author

Sounds good.👍
#204 should be included too

@yonaskolb yonaskolb added this to the 4.0.0 milestone Jan 22, 2018
@@ -58,17 +58,17 @@ public extension PBXProj.Objects {
/// - toGroup: parent group
/// - options: additional options, default is empty set.
/// - Returns: all new or existing groups in the path and their references.
public func addGroup(named groupName: String, to toGroup: PBXGroup, options: GroupAddingOptions = []) -> [(reference: String, group: PBXGroup)] {
public func addGroup(named groupName: String, to toGroup: PBXGroup, options: GroupAddingOptions = []) -> [ObjectReference<PBXGroup>] {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, just thinking. This could either be [String: PBXGroup] or [ObjectReference<PBXGroup>]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think array is better here, to make it is easier to inspect all created groups in order that will match their path order. Also I think with dictionary it will be impossible to get the last created group which is likely to be used in other methods as input parameter.

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