-
Notifications
You must be signed in to change notification settings - Fork 62
fix duplicate name error warning logic #821
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
base: master
Are you sure you want to change the base?
fix duplicate name error warning logic #821
Conversation
dqthai
left a comment
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.
Looks good to me
|
Thanks for this.
|
|
@christianp Around line 2005, I subscribed to the Any additional feedback is welcomed. I can also remove the comments if you think they aren't necessary. |
|
@IsaacJrTypes Thanks. This isn't quite correct: it only sets the error message on the first I'll add some aesthetic comments to the code as well. |
| group.forEach(function(n) { | ||
| n.v.duplicateNameError(group.length > 1 ? n.name : null); | ||
| group.forEach(function (n) { | ||
| n.v.duplicateNameError(null); |
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.
While it won't noticeably affect performance, you can avoid having two nested forEach calls by setting n.v.duplicateNameError(null) when you loop over each name, instead of here where you're looping over names inside groups.
|
|
||
| // Finally, mark duplicate names | ||
| names.sort(Numbas.util.sortBy('name')); | ||
| var groupArray = []; |
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 think it's a good idea to include the type of a variable in its name. This could just be called groups.
| groupArray.forEach(function (group) { | ||
| var groupVariable = group[0]; | ||
| if (group.length > 1 && groupVariable.name != "") { | ||
| groupVariable.v.duplicateNameError(groupVariable.name); |
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.
You're only setting duplicateNameError on the first Variable in the group. You should set it for each variable in the group.
Again, groupVariable unnecessarily includes the type of the variable in its name. Just group will do.

The following code fixes the duplicate warning logic. Previously, the duplicate error logic would sort the names of the entries in the names input field, turn them into variable objects, and group them. Each group would be processed by the handle_group function, which would trigger updating the duplicateNameError knock-out observable if there more than 1 object in the group. The issue with this method is if the user inputs
a,b,a, theagroup would trigger the duplicateNameError function, but once thebgroup is processed, the duplicateNameError observable updates tonull.I used an object to track if there were any duplicates. Each time the user updates input names, a new object is created, and if there are duplicates, it returns the name error message.
