-
Notifications
You must be signed in to change notification settings - Fork 31
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
Duplicated labels removal #530
Conversation
The |
src/js/models/panel_model.js
Outdated
var filtered_lbls = [] | ||
_.each(Object.values(filtered_lbls_map), function(idx){ | ||
filtered_lbls.push(labs[idx]) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working fine for me now, thanks.
I don't want to sound too picky, but I had a slightly hard time understanding the code, and I'm also not sure that using Object.values
will always preserve the order of the items (although it seems to be working).
I just wonder, since you're filtering, if it's clearer to use the filter
function like this?
// filter unique keys only to remove duplicates
var keys = new Set(labs.map(lbl => this.get_label_key(lbl)));
var filtered_lbls = labs.filter(lbl => keys.has(this.get_label_key(lbl)));
Apologies: I understand that this may not be a "better" way for everyone, and I don't want to make code review and PR submission any more time-consuming than it has to be. So please feel free to ignore this suggestion if it doesn't feel better for you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to sound too picky,
Don't worry, you have much more experience than me on Javascript !
However, I tested your changes but duplicates are not always removed. If labs
contains duplicate labels, then duplicates are not removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - I just realised that actually this code would be expected to remove ALL labels (since the keys for all of them are in that Set()
.
I think I must have not rebuild my JS code when I tried that and assumed my change was working!
I actually decided to google for a better way to do this (https://stackoverflow.com/questions/2218999/how-to-remove-all-duplicates-from-an-array-of-objects).
This code is concise but I'm not sure that it is very understandable?
var keys = labs.map(lbl => this.get_label_key(lbl));
var filtered_lbls = labs.filter((lbl, index) => index == keys.indexOf(this.get_label_key(lbl)));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet now works correctly.
It is definitely more concise and I think it could be understandable quite easily (filtering based on the index of the first occurrence of the value)
So it should be ok 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also removes the dirty Object.values()
, which I don't really like as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working fine for me now, thanks.
Fix issue #502 by removing duplicate labels.