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

Fix datalib collection import #4568

Merged
merged 2 commits into from Sep 6, 2017

Conversation

@martenson
Copy link
Member

commented Sep 6, 2017

and some refactoring of JS callback hell

martenson added some commits Sep 5, 2017

fix import of collections to datalibs
I swear I fixed this before
'Close' : function() {Galaxy.modal.hide();}
}
});
var that = this;

This comment has been minimized.

Copy link
@dannon

dannon Sep 6, 2017

Member

We have two approaches to referring to instantiated 'this' in Javascript, and I think for the most part, we have standardized on using 'self'. Would you mind using the same nomenclature here?

This comment has been minimized.

Copy link
@martenson

martenson Sep 6, 2017

Author Member

Changing it at one place seems pointless. Changing it everywhere seems like a project I do not want to do.

/client$ ack -hc 'var that'
45

This comment has been minimized.

Copy link
@dannon

dannon Sep 6, 2017

Member

I checked the count before commenting. 437 'self', so about 10x more commonly used in the codebase. Like any other refactoring, making small improvements as we rewrite code is a nice way to progress. I won't -1, of course, if you'd rather not. Someone's going to have to do it though.

This comment has been minimized.

Copy link
@martenson

martenson Sep 6, 2017

Author Member

I politely disagree, this does not have to be done.

This comment has been minimized.

Copy link
@dannon

dannon Sep 6, 2017

Member

Okay.

This comment has been minimized.

Copy link
@jxtx

jxtx Sep 6, 2017

Contributor

Soon we will have arrow functions and that should kill a lot of these. Not worth worrying about in the meantime.

(put my comment in the wrong place)

This comment has been minimized.

Copy link
@dannon

dannon Sep 6, 2017

Member

We've made a lot of steps towards code style standards lately, and this was a suggestion of an incremental step towards standards that we use across the codebase. This file, in particular, uses both paradigms from method to method, and it'd certainly be easier to read if it just picked one -- I just have to say I'm pretty surprised by the pushback here.

That we'll have arrow functions soon is a good point, and will definitely help as we're able to update all these, but that's a much larger project that I'd be blown away if we accomplish across much of the javascript codebase in the next few months. That project would also arguably benefit from us being able to quickly grep for instance methods using a standard approach.

Anyway, wasn't looking for an argument here, just a minor suggestion. Merging anyway.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 7, 2017

Member

@martenson I'm not going to hold up future PRs or anything - I find these JS PRs particularly problematic to revisit because of the packed script issue. But I do find the JavaScript quite overwhelming and a part of that is varying spacing and naming conventions across the codebase. Can we agree we don't need to revisit things for this PR but for future code use self because that is more common? I suspect I've used both in the past depending on where I was - but it would be good to agree to stick with on IMO - it would help me be more confident about what I was reading and writing I think.

This comment has been minimized.

Copy link
@martenson

martenson Sep 7, 2017

Author Member

@jmchilton @dannon Will do. The spacing problem should go away with the webpack upgrade I expect.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

Minor refactoring suggestion above, but this looks like a good idea.

@galaxybot galaxybot added this to the 17.09 milestone Sep 6, 2017

@dannon dannon merged commit 20b9356 into galaxyproject:dev Sep 6, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 290 tests run, 4 skipped, 0 failed.
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 44 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@martenson martenson moved this from Done to Closed in Data Libraries Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.