breautek commented on code in PR #143:
URL:
https://github.com/apache/cordova-node-xcode/pull/143#discussion_r1662495849
##########
lib/pbxProject.js:
##########
@@ -1644,6 +1644,10 @@ function pbxShellScriptBuildPhaseObj(obj, options,
phaseName) {
obj.shellPath = options.shellPath;
obj.shellScript = '"' + options.shellScript.replace(/"/g, '\\"') + '"';
+ if (options.alwaysOutOfDate != null) {
Review Comment:
The current codebase uses the triple equality and we should continue this
pattern for consistency.
This will more strictly test against `null` without type coercion (though
functionality I believe in this case it isn't any different than `!=`).
```suggestion
if (options.alwaysOutOfDate !== null) {
```
##########
test/addBuildPhase.js:
##########
@@ -194,4 +194,26 @@ exports.addBuildPhase = {
test.equal(buildPhase.shellScript, '"echo \\"hello world!\\""');
test.done();
},
+ 'should add the PBXBuildPhase with alwaysOutOfDate property': function
(test) {
+ var options = {
+ shellPath: '/bin/sh',
+ shellScript: 'test',
+ alwaysOutOfDate: true
+ };
+
+ var buildPhase = proj.addBuildPhase([], 'PBXShellScriptBuildPhase',
'Run a script', proj.getFirstTarget().uuid, options).buildPhase;
+ test.equal(buildPhase.shellPath, '/bin/sh');
+ test.equal(buildPhase.shellScript, '"test"');
+ test.equal(buildPhase.alwaysOutOfDate, true);
Review Comment:
The way this test is conducted this will pass but was this tested against a
real project?
In my xcode projects, xcode will use `1` or `0` values instead of true and
false. I'm not sure if our writer will translate booleans to 0/1 when printed
to file, or if it will print as "true", I'm also not sure if xcode will accept
a "true" value over `1`.
For the sake of "self-documenting" maybe the test should match the value
types that is used by xcode?
##########
test/addBuildPhase.js:
##########
@@ -194,4 +194,26 @@ exports.addBuildPhase = {
test.equal(buildPhase.shellScript, '"echo \\"hello world!\\""');
test.done();
},
+ 'should add the PBXBuildPhase with alwaysOutOfDate property': function
(test) {
+ var options = {
+ shellPath: '/bin/sh',
+ shellScript: 'test',
+ alwaysOutOfDate: true
+ };
+
+ var buildPhase = proj.addBuildPhase([], 'PBXShellScriptBuildPhase',
'Run a script', proj.getFirstTarget().uuid, options).buildPhase;
+ test.equal(buildPhase.shellPath, '/bin/sh');
+ test.equal(buildPhase.shellScript, '"test"');
+ test.equal(buildPhase.alwaysOutOfDate, true);
Review Comment:
Also other places in the test will use the integers to represent the boolean
as well:
https://github.com/apache/cordova-node-xcode/blob/master/test/addBuildPhase.js#L61
so using ints will also make the test more consistent with other parts of the
codebase, even if `true`/`false` is accepted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org
For additional commands, e-mail: issues-h...@cordova.apache.org
