-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: allow extension of helper app plists with extendHelperInfo #1233
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1233 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 716 729 +13
=========================================
+ Hits 716 729 +13
Continue to review full report at Codecov.
|
4f7bf7f
to
cde6451
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.
I made some readability suggestions / nits but nothing major. Overall looks reasonable.
const updateIfExists = [ | ||
['helperRendererPlist', '(Renderer)', true], | ||
['helperPluginPlist', '(Plugin)', true], | ||
['helperGPUPlist', '(GPU)', true], | ||
['helperEHPlist', 'EH'], | ||
['helperNPPlist', 'NP'] | ||
] | ||
|
||
for (const [plistKey] of [...updateIfExists, ['helperPlist']]) { |
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 first arg of each array in updateIfExists
is used as a key. Maybe this variable should be an object instead of an array of arrays e.g.
const updateIfExists = {
'helperRendererPlist': ['(Renderer)', true],
'helperPluginPlist': ['(Plugin)', true],
'helperGPUPlist': ['(GPU)', true],
'helperEHPlist': ['EH'],
'helperNPPlist': ['NP']
}
Not a big deal but it would make the code that uses it a little easier to read
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's used as a key on a different object, I think this would be harder to read because you'd have to pull the keys and values separately via entries()
or with two calls two keys()
and values()
. Also code hasn't been introduced here, in the spirit of minimal and related changes we probably shouldn't refactor here.
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'll take a closer look once Charles's comments are addressed.
cde6451
to
665c420
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.
This looks good conceptually. Just some cleanup needed, then we should be good to merge.
Hi @MarshallOfSound , This looks great! Can you supply instructions on how to do so? |
For apps that want to customize the Helper app plist files they currently have to do so in an
afterCopy
or other out-of-band step. AddingextendHelperInfo
to matchextendInfo
makes it much easier to extend these plist files.This PR also ensures we set
CFBundleShortVersionString
andCFBundleVersion
on helper plists.