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

Fix: adding a project to a workspace is broken on 0.17.0 #155

Merged
merged 4 commits into from
May 21, 2014
Merged

Fix: adding a project to a workspace is broken on 0.17.0 #155

merged 4 commits into from
May 21, 2014

Conversation

alessandroorru
Copy link
Contributor

Since version 0.17.0, workspace.rb uses new FileReference system instead of string paths (#150), but there was a bug when executing “<<“ method:

/Library/Ruby/Gems/2.0.0/gems/xcodeproj-0.17.0/lib/xcodeproj/workspace.rb:75:in `<<': 
     undefined method `path' for "MyProject.xcodeproj":String (NoMethodError)

Method “<<“ was missing a conversion from the input string path to a FileReference instance.

Method “<<“ was missing a conversion from string path to FileReference
@alessandroorru alessandroorru changed the title Fixes adding a project to a workspace Fix: adding a project to a workspace is broken on 0.17.0 May 21, 2014
@fabiopelosin
Copy link
Member

Looks good, can you add a test and an entry to the Changelog?

@alessandroorru
Copy link
Contributor Author

@irrationalfab Sure, I'm working on it. But which version of rspec should I use? I'm using rspec 2.14.8 but I'm getting some misbehavior in workspace_spec.rb. Moreover, in Gemfile there aren't references to rspec.
PS I'm almost new to Ruby so be free to tell me that my doubts are stupid ;)

@fabiopelosin
Copy link
Member

Xcodeproj uses bacon, see the rake spec task.

@alessandroorru
Copy link
Contributor Author

Ok thank you and sorry for the stupid question.
Test was already there, it was the "from new" test, but it was bugged too and it didn't fail.

Can you check if change log is ok?

@fabiopelosin
Copy link
Member

Looks good to me, usually to prevent merge conflicts we use the ## master header as a version. Did you decide to not credit yourself on purpose?

@alessandroorru
Copy link
Contributor Author

Ok, done! thank you for your support ;)

@fabiopelosin
Copy link
Member

Ace!

fabiopelosin added a commit that referenced this pull request May 21, 2014
Fix: adding a project to a workspace is broken on 0.17.0
@fabiopelosin fabiopelosin merged commit 46542ab into CocoaPods:master May 21, 2014
@fabiopelosin
Copy link
Member

Thank you for the patch 🍻

@alessandroorru
Copy link
Contributor Author

@irrationalfab I have just found out that this bug causes a bigger problem: if you add a project with the bugged << and then try to save the workspace, the workspace file gets totally empty. Maybe it should be better to add a validation before the actual write to disk

@fabiopelosin
Copy link
Member

The best way would be to add validation and coercion to the method:

def <<(proj)
  proj = proj.path if proj.is_a?(Project)
  unless proj.is_a?(String)
    raise ArgumentError, "The proj `#{}` parameter must be either a string or a project"
  end
  # ...
end

(untested code)

@alessandroorru
Copy link
Contributor Author

Yes, this is ok, but what if other parts of code (not the << method) create a corrupt version of the workspace without errors? When workspace gets saved, exceptions get thrown and the saved workspace file is cleared. I think there should be some kind of checks there too

@fabiopelosin
Copy link
Member

I think that it would be simpler to guard the methods that edit the workspace, like done in the above example.

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