Conversation
FelipeDefensor
left a comment
FelipeDefensor
left a comment
There was a problem hiding this comment.
This is a clear improvement over my implementation. The only thing, apart from the comments, is that I am not sure we want to silently "adjust" conflicting IDs in the long term. A user might, for instance, write a script that creates a components with a hardcoded ID, and later use that ID to get the component for a future operation. That operation would misteriously fail if we changed the ID when creating the component without an error or warning. Since we are rather far from such an API in the moment, I am fine with going for what you propose, at least for now.
| next_id = next(self._id_counter) | ||
| return str(next_id) | ||
| if int_id in existing_ids: | ||
| return str(next(self._id_counter)) |
There was a problem hiding this comment.
next(self._id_counter) might already be in existing_ids, no?
There was a problem hiding this comment.
No, see test_create_component_with_higher_id_before_lower_id. self._id_counter always increments from the largest input id, so there's no way it'll be smaller.
| if not existing_ids or int_id > max(existing_ids): | ||
| self._id_counter = itertools.count(int_id + 1) |
There was a problem hiding this comment.
Much better than what we had.
also removes scanning through all of the current ids if no id is given.
2 participants
also removes scanning through all of the current ids if no id is given.