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

[wip] Remove CoffeeScript ref variables #4833

Closed
wants to merge 6 commits into from

Conversation

kevinsawicki
Copy link
Contributor

  • Cleans up the ref, ref1, ref2, etc. variables left over from the CoffeeScript migration.
  • Migrates to using const and let over long var lines at the top of several functions.
  • Migrates to using forEach, find, filter, map over for loops in several places.

Before

BrowserWindow.fromWebContents = function(webContents) {
  var i, len, ref1, window, windows;
  windows = BrowserWindow.getAllWindows();
  for (i = 0, len = windows.length; i < len; i++) {
     window = windows[i];
     if ((ref1 = window.webContents) != null ? ref1.equal(webContents) : void 0) {
       return window;
     }
   }
};

After

BrowserWindow.fromWebContents = function(webContents) {
  return BrowserWindow.getAllWindows().find(function(window) {
    const contents = window.webContents;
    return contents != null && contents.equal(webContents);
  });
};

Refs #4065

@kevinsawicki kevinsawicki changed the title Remove CoffeeScript ref variables [wip] Remove CoffeeScript ref variables Mar 16, 2016
@kevinsawicki
Copy link
Contributor Author

This pull request is somewhat challenging to review due to this type of change so I'm going to review it several times myself and to make sure no unintended behavior was changed.

if (pos > 0) {
for (i = j = ref1 = pos - 1; ref1 <= 0 ? j <= 0 : j >= 0; i = ref1 <= 0 ? ++j : --j) {
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 decided to migrate this line given its lack of readability

@YurySolovyov
Copy link
Contributor

Since this PR is against master, can't you use rest args for arguments handling?

return app.on(name, function() {
var args, event, webContents;
event = arguments[0], webContents = arguments[1], args = 3 <= arguments.length ? slice.call(arguments, 2) : [];
['login', 'certificate-error', 'select-client-certificate'].forEach(function(name) {
Copy link
Member

Choose a reason for hiding this comment

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

We can probably use the for of syntax for iterating array, it is clearer and more efficient.

@kevinsawicki
Copy link
Contributor Author

I'm going to close this out for now and break this up into some smaller pull requests that focus on the following things individually:

  • Using rest parameters
  • Using destructured assignenment
  • Removing temp ref variables

@kevinsawicki kevinsawicki deleted the remove-coffeescript-refs branch March 18, 2016 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants