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

Make XcodeProj Writable #20

Merged
merged 13 commits into from
Jul 25, 2017
Merged

Make XcodeProj Writable #20

merged 13 commits into from
Jul 25, 2017

Conversation

yonaskolb
Copy link
Collaborator

@yonaskolb yonaskolb commented Jul 22, 2017

resolves #18

This makes XcodeProj writable

There are also some other changes bundled up here:

Removal of path property from top level objects.

Where something came from is not really an important part of its identity. The path where things are written has now been moved to the Writable protocol. This also allows one to change the top level path of something, without having to change all the child objects. (For example when writing XcodeProj to a new path)

Use PathKit for writing

Changed to use the simple API of PathKit to write files. The FileManager instance used for writing before was hardcoded anyway. There is still a dependancy injected FileManager that is used in the reading of files. This could be replaced with PathKit as well, removing quite a bit of code. What do you think @pepibumur?

@yonaskolb yonaskolb requested a review from pepicrft July 22, 2017 22:12
@yonaskolb yonaskolb force-pushed the feature/writable_xcproject branch from e21bc9c to 2cf0b79 Compare July 23, 2017 12:06
public func write(path: Path, override: Bool = true) throws {
try path.mkpath()

// write workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd create fileprivate methods instead of using comments.

@yonaskolb
Copy link
Collaborator Author

Oh, there's a failing test here, let me fix it. (And also rebase)

public static func == (lhs: XcodeProj, rhs: XcodeProj) -> Bool {
return lhs.workspace == rhs.workspace &&
lhs.pbxproj == rhs.pbxproj
//TODO: make SharedData equatable: lhs.sharedData == rhs.sharedData
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do this on a different PR, also conforming the protocol from XCSharedData.

@pepicrft
Copy link
Contributor

pepicrft commented Jul 25, 2017

Great job @yonaskolb, I left a couple of comments but in general looks ok. Answering your comments in the description:

Removal of path property from top level objects.

I agree with this point, it gives you the flexibility to decide where to write the model.

Use PathKit for writing

Sounds good! I passed the FileManager dependency to inject the behaviour for unit testing. However, I ended up testing the integration directly with the filesystem. If in the future we plan to unit-test it (besides the integration test) we can consider injecting a dependency.

@pepicrft
Copy link
Contributor

There seems to be a conflict in the XCWorkspace 😛

@yonaskolb yonaskolb force-pushed the feature/writable_xcproject branch from 0fae1ac to 2afdcc0 Compare July 25, 2017 10:57
@yonaskolb
Copy link
Collaborator Author

Should be good to go!

@pepicrft pepicrft merged commit 8a40faa into master Jul 25, 2017
@pepicrft pepicrft deleted the feature/writable_xcproject branch July 25, 2017 11:48
@pepicrft
Copy link
Contributor

Merging @yonaskolb

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.

Make XcodeProj writable
2 participants