Use camelCase identifiers in the JS API #580

j-f1 · 2017-02-07T01:08:55Z

I have read the section in CONTRIBUTING.md about pull requests.
My changes are appropriately documented.
My changes have sufficient test coverage.
- Does it need any tests?
The test suite passes.

This fixes #325.

malept

Thank you for the PR, I had no idea when I was going to get to this.

Some general comments:

I'm not really a fan of having a separate list of parameters that need to be maintained. I think what I want to do here is in the next major version, get rid of that and use the camelize module.
In the meantime, can we have a few tests for camelCase?

malept · 2017-02-07T01:16:10Z

common.js

+    Object.keys(module.exports.kebabProperties).forEach(function (key) {
+      var value = module.exports.kebabProperties[key]
+      if (properties.hasOwnProperty(key)) {
+        warn && warning(`The ${key} parameter is deprecated when used from JS, use ${value} instead`)


I would prefer it if this was a full-fledged if statement.

Also, it should be mentioned that the old property is going to be removed in the next major version.

malept · 2017-02-07T02:48:24Z

@j-f1 It looks like there's a problem in win32.js, take a look at the test output.

Also, what part of the testsuite is not running locally?

malept · 2017-02-07T08:50:22Z

Could you also add a NEWS entry? (An entry under both "Changed" and "Deprecated".)

j-f1 · 2017-02-09T12:36:26Z

@malept Sorry for the delay. I’ve added the changes requested.

The problem doesn’t make sense. Presumably, the files are minified somehow (see this line).

When I try to run the test locally, it downloads Electron, then nothing happens.

I’m also not exactly sure what to test for camelCase.

malept

The tests are still broken: https://travis-ci.org/electron-userland/electron-packager/jobs/199956707#L4331
For new tests, I would take one of the attributes that you know will be converted and directly test the camelCase function. (I guess in test/basic.js?)
Regarding your local tests problem: Try running the tests with the environment variable DEBUG="*" set, and paste the output somewhere accessible.

malept · 2017-02-09T16:28:20Z

NEWS.md

@@ -2,6 +2,12 @@

 ## Unreleased

+### Changed
+* The JS API now uses camelCase identifiers.


Options previously formatted in kebab-case (i.e., with hyphens) are now available in camelCase, per JavaScript naming standards (#580)

malept · 2017-02-09T16:29:25Z

NEWS.md

+* The JS API now uses camelCase identifiers.
+
+### Deprecated
+* Using kebab-case keys for the JS API is deprecated. Please use camelCase.


Options currently formatted in kebab-case (i.e., with hyphens) are deprecated in favor of their camelCase variants, per JavaScript naming standards (#580)

malept · 2017-02-09T16:32:39Z

docs/api.md

@@ -51,13 +51,13 @@ An array of functions to be called after Electron has been extracted to a tempor

 When `true`, sets both [`arch`](#arch) and [`platform`](#platform) to `all`.

-##### `app-copyright`
+##### `appCopyright`


For the docs, what I typically do when I'm renaming an option is add an entry for the old name with the type and a deprecated message (for example: version-string)

@malept Should I place the deprecated names right underneath the new one, or should I group them at the bottom?

Please keep the options alphabetized, so probably place the deprecated options somewhere above their respective new names.

malept · 2017-02-09T16:33:27Z

common.js

+    'helper-bundle-id': 'helperBundleId',
+    'osx-sign': 'osxSign',
+    'protocol-name': 'protocolName',
+    'version-string': 'versionString'


There's no point in camel-casing version-string, it's deprecated.

malept · 2017-02-09T16:51:31Z

The problem doesn’t make sense. Presumably, the files are minified somehow

I think this is an artifact of the code coverage module.

malept

I figured out what caused the test failure.

malept · 2017-02-09T16:53:01Z

win32.js

-      if (opts['app-copyright']) {
-        rcOpts['version-string'].LegalCopyright = opts['app-copyright']
+      if (opts.appCopyright) {
+        rcOpts.versionString.LegalCopyright = opts.appCopyright


This is incorrect, rcOpts is for the rcedit module. I take it you did a global search/replace?

malept · 2017-02-09T16:53:25Z

win32.js

      }

-      if (opts.icon || opts.win32metadata || opts['version-string'] || opts['app-copyright'] || opts['app-version'] || opts['build-version']) {
+      if (opts.icon || opts.win32metadata || opts.versionString || opts.appCopyright || opts.appVersion || opts.buildVersion) {


Revert the opts.versionString change, please.

malept · 2017-02-09T18:04:41Z

docs/api.md

 *Object* or *`true`*

 If present, signs OS X target apps when the host platform is OS X and XCode is installed. When the value is `true`, pass default configuration to the signing module. The configuration values listed below can be customized when the value is an `Object`. See [electron-osx-sign](https://www.npmjs.com/package/electron-osx-sign#opts) for more detailed option descriptions and the defaults.
 - `identity` (*String*): The identity used when signing the package via `codesign`.
 - `entitlements` (*String*): The path to the 'parent' entitlements.
 - `entitlements-inherit` (*String*): The path to the 'child' entitlements.

+
+


Unnecessary whitespace

malept · 2017-02-09T18:04:54Z

docs/api.md

+
+*String* (**deprecated** and will be removed in a future major version, please use the
+[`electronVersion`](#electronVersion) parameter instead)
+


This never existed as a JavaScript API option.

malept

I've squashed your commits, fixed up the NEWS entries, and added a separate test for camelCase. I'll merge once the tests run satisfactorily.

Thanks for your work on this!

malept requested changes Feb 7, 2017

View reviewed changes

malept added docs-needed 📝 Pull request needs documentation enhancement Feature request needs work 🚧 Feature request needs to be fleshed out before maintainers can take action on it tests-needed Pull request needs tests labels Feb 7, 2017

j-f1 force-pushed the camel-case-api branch from 31f260c to 671eb28 Compare February 9, 2017 12:25

malept requested changes Feb 9, 2017

View reviewed changes

j-f1 force-pushed the camel-case-api branch from 2a59c47 to 1d99e71 Compare February 9, 2017 17:12

j-f1 changed the title ~~Use camelCase identifiers in the JS API.~~ Use camelCase identifiers in the JS API Feb 9, 2017

malept requested changes Feb 9, 2017

View reviewed changes

j-f1 and others added 3 commits February 26, 2017 17:56

Use camelCase identifiers in the JS API

f686ced

Fix up NEWS entries

4c5edb2

Add test for the camelCase function

b4cac1b

malept force-pushed the camel-case-api branch from a913f78 to b4cac1b Compare February 27, 2017 01:57

malept approved these changes Feb 27, 2017

View reviewed changes

malept merged commit ae6f036 into electron:master Feb 27, 2017

malept mentioned this pull request Jun 12, 2017

Remove support for hyphenated option names via JS API #669

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use camelCase identifiers in the JS API #580

Use camelCase identifiers in the JS API #580

j-f1 commented Feb 7, 2017 •

edited

Loading

malept left a comment

malept Feb 7, 2017

malept Feb 7, 2017

malept commented Feb 7, 2017

malept commented Feb 7, 2017

j-f1 commented Feb 9, 2017 •

edited

Loading

malept left a comment

malept Feb 9, 2017

malept Feb 9, 2017

malept Feb 9, 2017

j-f1 Feb 9, 2017

malept Feb 9, 2017

malept Feb 9, 2017

malept commented Feb 9, 2017

malept left a comment

malept Feb 9, 2017

malept Feb 9, 2017 •

edited

Loading

malept Feb 9, 2017

malept Feb 9, 2017

malept left a comment


		String (deprecated and will be removed in a future major version, please use the
		[`electronVersion`](#electronVersion) parameter instead)

Use camelCase identifiers in the JS API #580

Use camelCase identifiers in the JS API #580

Conversation

j-f1 commented Feb 7, 2017 • edited Loading

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Feb 7, 2017

malept commented Feb 7, 2017

j-f1 commented Feb 9, 2017 • edited Loading

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Feb 9, 2017

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept left a comment

Choose a reason for hiding this comment

j-f1 commented Feb 7, 2017 •

edited

Loading

j-f1 commented Feb 9, 2017 •

edited

Loading

malept Feb 9, 2017 •

edited

Loading