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

Refactoring startApp to add callback as a parameter and removing redundant varaibles #14

Merged
merged 1 commit into from
Jun 18, 2014

Conversation

moizjv
Copy link
Member

@moizjv moizjv commented Jun 17, 2014

-Refactoring startApp to add callback as a parameter and removing redundant varaibles.
-Related to #2026

stopApp: true
});
// preventing null waitpkg
startAppOptions.waitPkg = startAppOptions.waitPkg || startAppOptions.pkg;
Copy link
Member

Choose a reason for hiding this comment

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

isn't this handled by the default case above?

Copy link
Member

Choose a reason for hiding this comment

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

Also you might want to clone before defaulting to avoid side effects.

startAppOptions = _.clone(startAppOptions);
_default(...

Copy link
Member Author

Choose a reason for hiding this comment

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

@jlipps no defaults only handles undefined. Earlier code had a comment for null so thought to handle it explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have cloned startAppOptions but still have null check in place , @jlipps any ideas if we still need the null check, since defaults will only handle undefined, it was explicitly commented about the null check.

@sebv
Copy link
Member

sebv commented Jun 18, 2014

@moiz, write your own default helper.

function cloneAndDefault(opts, defaults) {
  opts = _(opts).clone();  
  _(opts).each(function(val, key) { opts[key] = opts[key] || defaults[key];});
  return opts;
}

@moizjv
Copy link
Member Author

moizjv commented Jun 18, 2014

@sebv when values are set to false they would turn to true if default is true, so may be this looks better?

function cloneAndDefault(opts, defaults) {
  opts = _(opts).clone();  
  _(opts).each(function(val, key) { 
                  if(typeof opts[key] === "undefined" || opts[key] === null) {
                        opts[key]=defaults[key];}});
  return opts;
}

i was trying to tab in here so closed it by mistake :(

@moizjv moizjv closed this Jun 18, 2014
@moizjv moizjv reopened this Jun 18, 2014
@sebv
Copy link
Member

sebv commented Jun 18, 2014

_.isUndefined(val) || _.isNull(val)

@moizjv
Copy link
Member Author

moizjv commented Jun 18, 2014

I think _.each(defaults, function (val, key) { is correct

var cloneAndDefaults = function (opts, defaults) {
    opts = _.clone(opts);
    _.each(defaults, function (val, key) {
      if(_.isUndefined(opts[key]) || _.isNull(opts[key])){
        opts[key] = defaults[key];
      }
    });
    return opts;
  };

@moizjv
Copy link
Member Author

moizjv commented Jun 18, 2014

updated as per suggestions :)


var stop = stopApp ? "-S" : "";
ADB.prototype.startApp = function (startAppOptions, cb) {
var cloneAndDefaults = function (opts, defaults) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a generally useful function. Maybe we should include it up top or in a helper file. If we're just doing it here there's no need to make it a general function; we could just operate directly on startAppOptions.

jlipps added a commit that referenced this pull request Jun 18, 2014
Refactoring startApp to add callback as a parameter and removing redundant varaibles
@jlipps jlipps merged commit a61e652 into appium:master Jun 18, 2014
d0lfin added a commit to d0lfin/appium-adb that referenced this pull request Feb 9, 2016
kill process fo freeing port
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.

3 participants