-
Notifications
You must be signed in to change notification settings - Fork 458
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
Allow manipulation of existing xcscheme #288
Conversation
def initialize(target_or_node = nil) | ||
create_xml_element_with_fallback(target_or_node, 'BuildActionEntry') do | ||
native_target = target_or_node && target_or_node.is_a?(::Xcodeproj::Project::Object::PBXNativeTarget) | ||
is_test_target = native_target && target_or_node.product_type == 'com.apple.product-type.bundle.unit-test' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace magic strings by appropriate Constants::PRODUCT_TYPE_UTI
0866dd1
to
957c4d5
Compare
|
||
def build_configuration | ||
@xml_element.attributes['buildConfiguration'] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few other classes define methods like these as well. How about some common superclass or, if a class isn’t possible for some reason, some modules that define common behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought about a Mix-in but then I was afraid that it was a bit overkill (as this can be applied to other methods that are use in just 2 classes, etc). Will consider it for buildConfiguration anyway as this is used in more than 2 classes!
It’s looking real nice thus far 👍 |
module Xcodeproj | ||
# This class represents a Scheme document represented by a ".xcscheme" file | ||
# usually stored in a xcuserdata or xcshareddata (for a shared scheme) | ||
# folder. | ||
# | ||
class XCScheme | ||
XCSCHEME_FORMAT_VERSION = '1.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alloy Should I move that to Constants.rb
(even if that's only used in this class)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, yes
c3237ee
to
e638f0c
Compare
…heme elements and access it - WIP (specs KO)
…heme::XXXAction classes into one abstract parent class AbstractSchemeAction
…able to match XML node name
533a727
to
99c63d0
Compare
(+ fix access to private methods from specs)
99c63d0
to
eddfd69
Compare
Took me some time to add all the specs, but finally done! @alloy @segiddins This PR is now ready to be merged if you're ok with my code 😉 |
@doc.context[:attribute_quote] = :quote | ||
|
||
@scheme = @doc.elements['Scheme'] | ||
raise Informative, 'Unsupported scheme version' unless @scheme.attributes['version'] == Xcodeproj::Constants::XCSCHEME_FORMAT_VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so? Would you rather only have a warning "this version of XCScheme is unknown and new features may not be supported"? (But what if Apple uses a version 2.0 that breaks compatibility one day?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Xcode 7 updates this to 1.8 from 1.3. And they're nearly identical. We need to be backwards compatible, and ideally forwards compatible as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed, didn't know about the 1.8 version of Xcode 7. Then I would suggest that:
- we convert that exception to a simple warning for now (or totally silent it?), to merge that request and avoid piling up work on that feature
- then do some more investigation in the difference between 1.3 and 1.8, checking if it breaks stuff (this will need some additional fixtures, etc)
- and finally we do another PR to do what's needed if those changes impact this implementation, or simply remove the warning if the version attribute is "1.8" and that format is already compatible with the current code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure we just shouldn't check the version, but I'll let @neonichu weigh in on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Xcode 7b4 just created two brand new projects (one in ObjC one in Swift) — with Unit Tests and UI Tests activated — and both were in version 1.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say let's leave this exception in here for now, then and cross the bridge of multiple versions once it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this 1.8 format doesn't have much difference with 1.3. I'll ditch the version check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as latest Xcode seem to sometimes use 1.8 which is retro-compatible with 1.3)
This is looking 👍 |
Allow manipulation of existing xcscheme
The PR aims to refactor
Xcodeproj::XCScheme
to make it possible to load and manipulate existing schemes (as so far, creating.xcscheme
files from scratch was the only option possible).Note: This refactoring is expected to avoid breaking the existing API. Only new features have been added to the existing
XCScheme
class, and the implementation of the existing API have been simply adapted to the new model while keeping the method signatures intact.Xcodeproj::XCScheme
to wrap xcscheme elements in nicer objectsXCScheme
instance by loading an existing.xcscheme
fileCocoaPods
, to ensure it does not break even if we missed some specs