-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add support for specifying a dict of infoplist values #47
Conversation
@@ -12,6 +13,36 @@ _IOS_APPLICATION_KWARGS = [ | |||
"launch_storyboard", | |||
] | |||
|
|||
def write_info_plists_if_needed(name, plists): |
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 know this precedes this change, but when do we expect to see more than 1 plist supplied?
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.
rules_apple
allows passing multiple plists and it coalesces them, so we're just supporting the same interface as the apple rules for consistency
already_written_plists.append(plist) | ||
|
||
if not already_written_plists: | ||
already_written_plists.append("@build_bazel_rules_ios//rules/test_host_app:Info.plist") |
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.
Worth adding a test covering the default behavior? Or not worthwhile because no app hosted tests would be launchable?
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.
they wouldn't be launchable without the default, so it's defacto covered by the existing test tests
|
||
Args: | ||
name: The name of the app target these infoplists are for. | ||
plists: A list of either labels or dicts. |
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.
Can you explain what labels
would look like and when it would be used? It would be the path to an infoplist file? It looks like you are exercising a single dict path in the test that was added. Worth adding a test that includes the other form? Or does that already have coverage?
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.
The label would either be a path to a source plist file, or the label of a generated file. Bazel in general doesn't understand paths, instead using labels to refer to files when instantiating rules, and every source file implicitly gets a label that matches its path
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.
just one comment regarding the key method
if not already_written_plists: | ||
already_written_plists.append("@build_bazel_rules_ios//rules/test_host_app:Info.plist") | ||
|
||
return already_written_plists + written_plists |
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 don't understand what this method returns as? A list of written plists? The doc does not specify it. This will also helps me understand the doc better cos I saw 3 things happen inside the method when doc says there are 2 things happened (from my view)
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.
It returns a list of labels to plists.
What thing do you think is missing from the doc?
already_written_plists.append(plist) | ||
|
||
if not already_written_plists: | ||
already_written_plists.append("@build_bazel_rules_ios//rules/test_host_app:Info.plist") |
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.
Does this need to be added in the case that a dict is supplied?
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.
Yes, to match cocoapods' behavior
This allows specifying entries directly in the BUILD file, rather than requiring users to write out a plist file themselves This will allow cocoapods-bazel to emit `app_spec.info_plist` specifications into BUILD files, keeping the behavior whereby no files outside of BUILD files need to be written
e726d09
to
c6c7169
Compare
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.
Nice
This allows specifying entries directly in the BUILD file,
rather than requiring users to write out a plist file themselves
This will allow cocoapods-bazel to emit
app_spec.info_plist
specifications into BUILD files, keeping the behavior whereby no
files outside of BUILD files need to be written