Reimplemented Git-FTP features after 489a60c merge

[FezVrasta]

Currently it supports:

• Git-FTP remote (scope) creation
• Git-FTP remote selection
• Git-FTP remote deletion

There is a lot to rewrite and fix, will wait for your first review to proceed.

[zaggino]

lets use type="normal" and type="ftp" so we can add other types in the future

[zaggino]

First review finished. Please fix the cases where you only modified the original formatting, because it's hard to review changes. Example:

                        Git.createRemote("origin", url)
                            .then(function () {
                                return Git.push("origin");
                            })
                            .then(resolve)
                            .catch(reject);

you changed to

                        Git.createRemote("origin", url)
                        .then(function () {
                            return Git.push("origin");
                        })
                        .then(resolve)
                        .catch(reject);

Please never do this. If you have some kind of extension which does this automatically - turn it off.

[FezVrasta]

About the formatting problems, if I use your method the linter tells me that it expects } at 25 and instead is at 27...

[zaggino]

Which linter? Do you have JSHint installed?

[zaggino]

JSHint reads configuration from this file https://github.com/zaggino/brackets-git/blob/master/.jshintrc so it shouldn't show any errors.

[FezVrasta]

Sorry my fault, nevermind ^^"

[FezVrasta]

Just a little question.

We have Git and GitFtp.
Should I rename GitFtp.getFtpRemotes to GitFtp.getRemotes to make it like Git.getRemotes or is better repeat the Ftp word both in the name of the class and in the name of the function?

[zaggino]

Yes, rename it. GitFtp.getFtpRemotes doesn't make any sense.

[zaggino]

You can't use .all in this situation. .all passes only if everything passes and fails when any of the promises fail. Correct usage would be:

Git.getRemotes().then(function(gitRemotes) {
  return GitFtp.getFtpRemotes().then(function (ftpRemotes) {
    return [gitRemotes, ftpRemotes];
  }).catch(function (err) {
     // GitFtp.getFtpRemotes failed - do nothing with err
    return [gitRemotes, null];
  });
}).catch(function (err) {
  ErrorHandler.showError(err, "Git.getRemotes failed");
}).spread(function (gitRemotes, ftpRemotes) {
  // ftpRemotes might be null if getFtpRemotes failed
});

[FezVrasta]

In this way them are ran one after the other, not together, and it makes the extension load in more time. Not sure this is the right solution...

[zaggino]

Ok, I'll make an API for them to run simultaneously, put there a TODO comment please.

[FezVrasta]

We need Promise.allSettled but looks like it's not available. Probably we should open an issue on Bluebird instead of write some tweak by ourselves.

[zaggino]

Btw why do you want to ignore an error? If I have ftps turned on, I don't want them to fail silently - I want to see an error dialog and submit a bug.

[FezVrasta]

Because is the shortest way I've found to make the GitFtp.getRemotes part pluggable without have to write two almost identical functions.

But now that you make me think about it maybe I could just write something like:

if(!gitFtpEnabled) {
    function getFtpRemotes() { return null; }
}
else {
    getFtpRemotes = GitFtp.getRemotes;
}
Promise.all([Git.getRemotes(), getFtpRemotes])

What do you think?

[FezVrasta]

Oh wait, found the solution I guess...
http://stackoverflow.com/questions/22658992/get-states-of-tasks-ran-by-bluebird-all-spread

[zaggino]

Why don't you just do this?

var promises = [];
promises.push(git.getremotes());
if (ftpEnabled) promises.push(ftp.getremotes())
Promise.all(promises).spread(remotes, ftpRemotes)

remotes will be always there and ftpRemotes will be there only if it was enabled

[FezVrasta]

Umh yes actualy in this way it's much better, thanks.

[FezVrasta]

Just a problem with your method, I can't know what failed of the two taks if one of them fails... Does it matter?

[zaggino]

No, it really doesn't matter, just do the normal handler maybe with an addition

var msg = "Getting the remotes failed";
if (ftpEnabled) msg += " (FTP enabled)";
ErrorHandler.showError(err, msg)

[FezVrasta]

Problem:

var promises = [];
    promises.push(Git.getRemotes());
    if (gitFtpEnabled) { promises.push(GitFtp.getRemotes()); }

is wrong, because I assign the result of the two functions to promises.

var promises = [];
    promises.push(Git.getRemotes);
    if (gitFtpEnabled) { promises.push(GitFtp.getRemotes); }

is even wrong because the resulting array is:

[function, function]
0: function getRemotes() 
1: function getRemotes() 

and not

[function, function]
0: function Git.getRemotes() 
1: function GitFtp.getRemotes() 

[zaggino]

The first case is right because Git.getRemotes() returns a promise - not a value. Have you tested it?

[FezVrasta]

yes, it doesn't work, I think the function is executed during the assignment (note the ())

[zaggino]

The function is executed and returns a promise which will be resolved later.

Git.getRemotes().then(function(remotes) { ... })

is exactly the same as

var promise = Git.getRemotes();
promise.then(function (remotes) { ... });

[zaggino]

Just put a TODO comment there I'll look at it - it has to work, I have done this before.

[zaggino]

Can I review again / merge this?

[FezVrasta]

As you prefer, I'm not at office so I will not work on it till tomorrow (EU time).
A review would be welcome I think.

[zaggino]

Why there's this check for gitFtpEnabled and again lower at line 15?

[zaggino]

I get it, nevermind.

[zaggino]

I'm reading the diff wrong or have this disappeared completely?

if (remotes.length > 0) {
 -                EventEmitter.emit(Events.GIT_REMOTE_AVAILABLE);
 -            } else {
 -                EventEmitter.emit(Events.GIT_REMOTE_NOT_AVAILABLE);
 -                clearRemotePicker();
 -                return;
 -            }

[zaggino]

use 4 spaces for indentation

[FezVrasta]

#288 (comment) looks like it's disappeared, I've almost replaced the entire Remotes.js with my old version.

[zaggino]

Ok, I'll merge this today so you can continue tomorrow.

[FezVrasta]

Thanks :)

[zaggino]

@FezVrasta done, see #294 for details.