Skip to content

Feature/priorityq jason#17

Merged
jay-tyler merged 5 commits intofeature/priorityqfrom
feature/priorityq_jason
Jul 5, 2015
Merged

Feature/priorityq jason#17
jay-tyler merged 5 commits intofeature/priorityqfrom
feature/priorityq_jason

Conversation

@jay-tyler
Copy link
Collaborator

All milestone issues complete.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider a different method other then len(item) == 2 to determine if this is a (val, priority) tuple here. As is, it will catch cases that we don't want.

For example, if a string of length 2 comes in as an item like PriorityQueue(['hi', (10, 0), ('something else', 1)]), it will potentially examine 'hi', find it has length 2 and then:

self.insert('h', 'i')

Which isn't what we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, look at the doc string. Iterables have to be packaged using the (val, priority) notation. I think it's reasonable. Let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Then why don't we just eliminate "A non-iterable value" and require all non-QNode objects to be passed in as a container? It will simplify and eliminate unintended effects like described above.

So, each item inside iterable can be either:

  • a QNode object
  • a tuple with value, priority (if priority not given, assume priority None)

I think we can be explicit here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll open this as a separate issue if we want to address it.

jay-tyler added a commit that referenced this pull request Jul 5, 2015
@jay-tyler jay-tyler merged commit a137018 into feature/priorityq Jul 5, 2015
@jonathanstallings jonathanstallings deleted the feature/priorityq_jason branch August 6, 2015 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants