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

feat: skipIdentityValidation #126

Merged
merged 4 commits into from
Feb 25, 2017
Merged

Conversation

develar
Copy link
Contributor

@develar develar commented Feb 24, 2017

Not documented because intended to be internal option for programmatic usage

Not documented because intended to be internal option for programmatic usage
platform?: string;
keychain?: string;
}
declare module "electron-osx-sign" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise typing is not used if module: es2015 Why es2015 module is used for node projects? Because it is transpiled by babel in the end :)

index.d.ts Outdated
'requirements'?: string;
'type'?: string;
version?: string;
skipIdentityValidation?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@develar I am not sure if to order the options alphabetically or just by priority/usage frequency with skipIdentityValidation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add properties by date — newest in the end. Or by group. You can reorder as you want :)

sign.js Outdated
@@ -219,6 +219,9 @@ var signAsync = module.exports.signAsync = function (opts) {
var promise
if (opts.identity) {
debuglog('`identity` passed in arguments.')
if (opts.skipIdentityValidation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if it would be better to have this option skipIdentityValidation as identity-validation (as in the way we treat gatekeeper-assess) so the parameters are more consistent across electron-osx-sign?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer camel-case, but you are right — I will rename to identity-validation for consistency.

Copy link
Contributor

@sethlu sethlu Feb 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@develar yea camel-case should work better with the programmable interface.

As a side question before publishing the module, would you prefer identity-validation or validate-identity as the option name?


I am thinking for future support, validate-identity may be more sustainable as the prefix validate- is generally reusable?

Ref: https://www.gnu.org/prep/standards/html_node/Option-Table.html#Option-Table

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Control question is: whether to option-name? Whether to validate identity? It is why originally name skipIdentityValidation was suggested. If we inverse and simplify — whether to validate identity.

Electron-builder has option npmRebuild, but npm here is a prefix. forceCodeSigning another example.

@sethlu sethlu merged commit 958f2b1 into electron:master Feb 25, 2017
@sethlu
Copy link
Contributor

sethlu commented Feb 25, 2017

Merged; thanks @develar! I will release a latest version later today.

@sethlu
Copy link
Contributor

sethlu commented Mar 1, 2017

@develar v0.4.4 already published on npm 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants