Skip to content

use list for unique_task2name #100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 1, 2020
Merged

use list for unique_task2name #100

merged 3 commits into from
Dec 1, 2020

Conversation

@dlashua
Copy link
Contributor

@dlashua dlashua commented Nov 30, 2020 •
edited
Loading

Fixes #99
@dlashua dlashua changed the title use list for unique_task2name (fixes #99) use list for unique_task2name Nov 30, 2020
craigbarratt
craigbarratt reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@craigbarratt craigbarratt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great. My only suggestion is to use a set instead of list, which is more efficient if we had a lot of names, and also add and discard don't require testing beforehand.
custom_components/pyscript/function.py Outdated
task = cls.unique_name2task[name]
if task in cls.unique_task2name:
if name in cls.unique_task2name[task]:
cls.unique_task2name[task].remove(name)
Copy link
Member

Choose a reason for hiding this comment

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

It would be more efficient to use a set, in which case the last two lines can just be a single-line discard (which is a no-op if not present)
custom_components/pyscript/function.py Outdated
cls.unique_name2task[name] = curr_task
cls.unique_task2name[curr_task] = name
if curr_task not in cls.unique_task2name:
cls.unique_task2name[curr_task] = []
Copy link
Member

Choose a reason for hiding this comment

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

This would be set() instead of []
@dlashua
Copy link
Contributor Author

dlashua commented Dec 1, 2020

I considered a set because it makes the values unique automatically. But, having never used one before, when I looked up the differences between a list and set I read that items in a set could not be changed or replaced. So I went with a list because I knew it would work (i.e. I chickened out on having to debug it if it didn't work as I expected).

I'll convert to a set.
@craigbarratt
Copy link
Member

set elements have the same restriction as dict keys - they have to be immutable (so their hashed value cannot change). So set elements and dict keys can't be things like lists, sets, dicts, or objects, since they can be changed.

A set is just like a dict where there isn't a value and the set members are the dict keys.
@craigbarratt craigbarratt merged commit 50ced30 into custom-components:master Dec 1, 2020
@craigbarratt
Copy link
Member

Looks great; fixes #99.
@dlashua
Copy link
Contributor Author

dlashua commented Dec 1, 2020

A set is just like a dict where there isn't a value and the set members are the dict keys.

well... TIL about object hashes in python. I've never tried to use a dict as a dict key before, but now I know why I can't. Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@dlashua @craigbarratt