-
Notifications
You must be signed in to change notification settings - Fork 351
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
Change the error message in model.py : top bricks with the same name ret... #602
Conversation
…returns the first occurrences too
Very nice initiative! I will have some remarks about the code, otherwise I think it is a very nice idea to have a more verbose message here. |
# find the first occurences and return it in the error message | ||
liste_name = [b.name for b in self.top_bricks] | ||
occ = liste_name[numpy.argmax([liste_name.count(liste_name[i]) | ||
for i in xrange(len(liste_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.
- liste_name -> list_of_names or name_list
- occ -> repeated_name
- the counting should be much easier to implement with collections.Counter. You should be interested in its
most_common
method. Also it can be used instead of
len(set(b.name for b in self.top_bricks)) < len(self.top_bricks):
with the advantage that it gives you the name of the duplicate. - the space that you put in the error message after the
same name
and before colon is a little bit non-standard. I think it's better to have writesame 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.
It's pretty redundant to call it list_of...
of ..._list
actually. Just names
will do.
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.
Agreed :)
if len(set(b.name for b in self.top_bricks)) < len(self.top_bricks): | ||
raise ValueError("top bricks with the same name") | ||
# check there is no occurence in top bricks name | ||
names = Counter([b.name for b in self.top_bricks]) |
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 might be an easier way to get all the names that are not unique, instead of just one:
names = set()
repeated_names = [brick.name for brick in self.top_bricks
if brick.name in names or names.add(brick.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.
names = Counter([brick.name for brick in self.top_bricks])
repeated_names = [name for name, count in names.items() if count > 1]
works too of course
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 would rather go the second option: having things done in an or
operand reminds me of sport programming in C++ and make me shiver.
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 agree. The first one is just a bad habit, faster but ugly.
By the way I have made the tests on my own code, do you need a test or something is already implemented ? |
@@ -155,8 +155,12 @@ def __init__(self, outputs): | |||
for brick in bricks: | |||
if brick not in children and brick not in self.top_bricks: | |||
self.top_bricks.append(brick) | |||
if len(set(b.name for b in self.top_bricks)) < len(self.top_bricks): | |||
raise ValueError("top bricks with the same name") | |||
# check there is no occurence in top bricks 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.
Not sure what you mean by "occurrence" here, but you can remove the comment, it's pretty clear from the code what it does.
repeated_names = [name for name, count in names.items() if count > 1] | ||
if repeated_names: | ||
raise ValueError("top bricks with the " | ||
"same name : {}".format(repeated_names)) |
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 think @rizar commented on this already, but it makes more sense to do "same name: {}"
and since we're nitpicking anyway, "same name: {}".format(', '.join(repeated_names))
would be even nicer.
I don't think a test is needed. Testing error messages gets kind of messy anyway, because you need to do string matching or something. Besides my last comment it looks ready to merge IMO. |
Done 2015-04-28 14:35 GMT-04:00 Bart van Merriënboer notifications@github.com:
|
Great, but with the latest change line 162 is one character too long... (Also, it still says |
Travis error seems to be not related to the PR, merging. |
Change the error message in model.py : top bricks with the same name ret...
I was struggling to understand what was wrong in my code as I put differents names for every parameters in my blocks model. Eventually It turns out that this error message is about the main bricks and not the parameters inside. I just thought it would help to add the first name where some occurrences happened. Maybe it is already handled in some ticket but in case there is not, it would be great to add such precision in the error message for beginners.
Have a nice day