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 PBXFileReference return an optional full path #12

Conversation

mbernson
Copy link

The filesystem path to a PBXReference may not always exist in the fullFilePaths
dictionary, for example when the user incorrectly edits (or merges) their Xcode project file.

I would like to catch this error in R.swift, but can't because this dictionary lookup in Xcode.swift is already a fatal error.

The file path to a PBXReference may not always exist in the `fullFilePaths`
dictionary, for example when the user incorrectly merges their Xcode project
file.
@mbernson mbernson changed the title Make PBXFileReference return an optional full path Make PBXFileReference return an optional full path Mar 16, 2016
@mac-cain13
Copy link
Collaborator

Little bit of background; We crashed into this forced unwrap in a project we worked on. It was an incorrectly merged file, but R.swift now exited with some sort of segfault complaining about a "mac architecture thing". Very confusing.

I don't feel Xcode.swift should support all kind of corrupt Xcodeproj files, but by making this an optional R.swift could indeed catch nil values here and warn the user about them.

@tomlokhorst: Do you agree this is an okay change?

@tomlokhorst
Copy link
Owner

As you may have noticed in the source code; This isn't the only !, there's a lot of them!

In practice, I've never seen Xcode.swift crash on any of them. Except for when the xcodeproj is corrupt, like you have here.

In general, I think it's a bad idea to "hide" errors like this.
By making fullPath optional, the user who has a non-corrupt xcodeproj now has to deal with an extra case that will never occur.

A different approach would be to have the XCProjectFile initializer do a bunch of checks to see if the xcodeproj is internally consistent.
It would be really nice if it would throw ProjectFileError.InternalInconsistency with something like:

- BuildFile (46880B8819C43A87006E1F66) references missing fileRef 3EACC98E19EE6D4300EB3C5E
- FileReference `AppDelegate.cpp` (288765A40DF7441C002DB57D) is not used in any BuildFile.

Perhaps a merge conflict?

@mac-cain13
Copy link
Collaborator

I agree with @tomlokhorst. After some IRL discussion I think a constructor that validates the project and throws clear errors is better than just making this an optional.

@mbernson do you have the time and would you like to take this up and PR that? Would be awesome if you have the time to pick this up. :)

@mbernson
Copy link
Author

Yes, I'd like to pick this up! But I can't give any guarantees for when it will be done.

@tomlokhorst
Copy link
Owner

I've created an issue for this: #13

@mbernson mbernson deleted the feature/pbxfilereference-optional-path branch March 20, 2016 10:21
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