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 lgtm alerts #4421

Merged
merged 5 commits into from Aug 16, 2017

Conversation

Projects
None yet
5 participants
@xiemaisi
Copy link
Contributor

commented Aug 14, 2017

This fixes a few minor JavaScript issues found by lgtm; for more details see https://lgtm.com/projects/g/galaxyproject/galaxy/alerts.

All fixes in this PR are code quality/maintainability fixes, and should not change behaviour in any way. Please let me know if you would like me to run any specific tests to make sure.

(Full disclosure: I work an lgtm's JavaScript analysis.)

@galaxybot galaxybot added the triage label Aug 14, 2017

@galaxybot galaxybot added this to the 17.09 milestone Aug 14, 2017

@xiemaisi xiemaisi force-pushed the xiemaisi:fix-lgtm-alerts branch from 89c9406 to de6c5d0 Aug 14, 2017

@xiemaisi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

Rebased to fix a conflict, and re-ran make client.

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

Thank you very much for this contribution @xiemaisi, please give us a few days to review as it is touching many parts of the app. Also do not worry about rebasing etc. - we will handle this unfortunate static relict.

@@ -4,6 +4,9 @@ define(['mvc/workflow/workflow-globals'], function( Globals ) {
this.isCollection = true;
this.rank = collectionType.split(":").length;
}

var ANY_COLLECTION_TYPE_DESCRIPTION, NULL_COLLECTION_TYPE_DESCRIPTION;

This comment has been minimized.

Copy link
@dannon

dannon Aug 16, 2017

Member

Technically correct (and an improvement), but I think it might be more clear to add the declaration with var below where they're defined.

This comment has been minimized.

Copy link
@xiemaisi

xiemaisi Aug 16, 2017

Author Contributor

I put them here to precede their first uses (which come before the definitions).

This comment has been minimized.

Copy link
@dannon

dannon Aug 16, 2017

Member

Yep. Maybe the best fix is to combine both concerns and reorder the definitions as well.

This comment has been minimized.

Copy link
@xiemaisi

xiemaisi Aug 16, 2017

Author Contributor

OK, I can do that.

@@ -12,7 +12,7 @@ $.ui.plugin.add("draggable", "scrollPanel", {
panel_pos = panel.position(),
panel_w = panel.width(),
panel_h = panel.height()
viewport = panel.parent();
viewport = panel.parent(),

This comment has been minimized.

Copy link
@dannon

dannon Aug 16, 2017

Member

Looks like this file needs a little more love; @xiemaisi should LGTM have added a comma after the line above?

This comment has been minimized.

Copy link
@xiemaisi

xiemaisi Aug 16, 2017

Author Contributor

Ah, indeed, my change here doesn't actually fix the alert. Will update.


// Render each chromosome's data.
chroms_data_layout = _.map(layout_and_data, function(chrom_info) {
_.each(layout_and_data, function(chrom_info) {

This comment has been minimized.

Copy link
@dannon

dannon Aug 16, 2017

Member

I may be missing it, but it looks like this is a breaking change. Instead of mapping onto a new array, this does _.each, but it doesn't actually manipulate the existing array, so no changes are made.

This comment has been minimized.

Copy link
@xiemaisi

xiemaisi Aug 16, 2017

Author Contributor

The array this used to map into was never used anywhere, hence I removed it and replaced _.map with _.each. If this function is, in fact, side effect-free (which I'm not in a position to judge), then the entire call can be removed.

This comment has been minimized.

Copy link
@dannon

dannon Aug 16, 2017

Member

Huh, yeah, I made the bad assumption that map() was intentional and that we needed that array later, but it definitely doesn't look like as I dig a little more. This is indeed probably better as _.each.

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Sorry I didn't group my comments as an actual review, I'm rusty at this! :)

Really nice pass cleaning up the codebase; I added commentary on a few things above, and am happy to +1/merge it once those are addressed. As mentioned by marten, don't worry about the conflicts and rebasing/rebuilding, we can do that at merge time.

xiemaisi added some commits Aug 11, 2017

Add missing `var` declarations.
In all these cases, a variable was not declared to be a local using `var` (and hence was implicitly interpreted as a global variable), but all uses of that variable were preceded by a definition within the same function, suggesting that the variable was, in fact, meant to be local, not global.
Remove an unused variable.
The result of the `_.map` call was never used, so I replaced it with `_.each`.

@xiemaisi xiemaisi force-pushed the xiemaisi:fix-lgtm-alerts branch from de6c5d0 to 57c6284 Aug 16, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@dannon I am a bit reluctant to merge this before the 17.09 given how many places are being touched. Also the nature of the var scope fixes can easily have unexpected effects that are hard to test. I would prefer to have this merged after the freeze later this month.

@xiemaisi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

Addressed suggestions by @dannon and rebased away conflicts.

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@martenson Just reluctant, or feel strongly about not merging it prior to 17.09? I think I'm pretty comfortable with the changes here. (and I think I'd actually prefer it in the freeze -- getting it more testing and making any patches we have to make easier to apply correctly)

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@dannon Just reluctant, of course feel free to merge.

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Ok, cool. I'll keep the scope change concerns in mind should we notice any weirdness during the freeze testing, and can volunteer to fix any fallout :)

@dannon dannon merged commit 1b7f25e into galaxyproject:dev Aug 16, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@dannon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Thanks again, @xiemaisi!

@xiemaisi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

My pleasure, @dannon.

I should point out that we offer GitHub integration so you can get notifications about new alerts in PRs before they are merged, see https://lgtm.com/docs/lgtm/using-lgtm-analysis-continuous-integration for details.

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@xiemaisi I enabled it for this repo, thanks. Are there any plans to make this integration 'official' in GitHub?

@sjvs

This comment has been minimized.

Copy link

commented Aug 16, 2017

@martenson: great to see that @xiemaisi has been able to help you out with some fixes from lgtm.com! My colleague Max asked me to have a look at your question.

What do you mean exactly by 'official' in GitHub — do you mean available through the GitHub marketplace? We are definitely working on that. There are some technicalities on GitHub's side that make this a little bit harder than you'd expect. For example: the new APIs are not available in GitHub Enterprise yet. So watch this space!

Any other suggestions for improvements are more than welcome. We've only recently launched lgtm.com and are deploying improvements almost daily. Input from early adopters like yourself will help making things even more awesome 😉!

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

@sjvs yeah I guess marketplace is the thing. Basically so it appears in this menu and has github-side configuration available.
screenshot 2017-08-16 13 20 31

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.