Skip to content
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

Use unicode slugify #19192

Merged
merged 20 commits into from Dec 17, 2018
Merged

Use unicode slugify #19192

merged 20 commits into from Dec 17, 2018

Conversation

pvizeli
Copy link
Member

@pvizeli pvizeli commented Dec 11, 2018 •
edited

Description:

Use mozilla slugify library with UTF-8 support:
https://github.com/mozilla/unicode-slugify

Fix: #19184

Update:
We need to move to https://github.com/un33k/python-slugify because the Mozilla PyPI packages are very outdated

@pvizeli pvizeli requested a review from a team as a code owner December 11, 2018 15:31
@homeassistant homeassistant added cla-signed core small-pr PRs with less than 30 lines. labels Dec 11, 2018
@ghost ghost assigned pvizeli Dec 11, 2018
@ghost ghost added the in progress label Dec 11, 2018
balloob
balloob reviewed Dec 11, 2018
View reviewed changes
homeassistant/util/__init__.py Outdated
@@ -14,6 +14,8 @@
from typing import (Any, Optional, TypeVar, Callable, KeysView, Union, # noqa
Iterable, List, Dict, Iterator, Coroutine, MutableSet)

from slugify import slugify
Copy link
Member

Choose a reason for hiding this comment

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

I sense some naming issues ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed :D

@amelchio
Copy link
Contributor

I definitely have localized entity ids that could be improved.

Kinda hard to give comments though, some before/after examples would be nice ... maybe as tests?

@Anonym-tsk
Copy link
Contributor

@pvizeli Please test results with this method https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/core.py#L78

balloob
balloob reviewed Dec 11, 2018
View reviewed changes
homeassistant/util/__init__.py Outdated

return text
return moz_slug.slugify(
text, ok='_-', space_replacement='_', only_ascii=True)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think - is a valid entity id ?

@balloob
Copy link
Member

balloob commented Dec 11, 2018

Breaking change because any entity that does not have a unique ID and has foreign characters in their name will get a new entity ID.

@pvizeli
Copy link
Member Author

pvizeli commented Dec 12, 2018 •
edited

Maybe we need to move to https://github.com/un33k/python-slugify because the Mozilla PyPI packages are very outdated

@pvizeli pvizeli changed the title RFC: Use mozilla slugify Use unicode slugify Dec 13, 2018
@balloob balloob mentioned this pull request Dec 14, 2018
Closed
3 tasks
balloob
balloob approved these changes Dec 14, 2018
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge when tests and typing fixed.

houndci-bot
houndci-bot reviewed Dec 16, 2018
View reviewed changes
tests/util/test_init.py Outdated Show resolved Hide resolved
tests/util/test_init.py Outdated Show resolved Hide resolved
tests/components/sensor/test_filesize.py Outdated Show resolved Hide resolved
tests/components/sensor/test_filesize.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/sensor/test_awair.py Outdated Show resolved Hide resolved
tests/components/device_tracker/test_meraki.py Outdated Show resolved Hide resolved
@pvizeli pvizeli merged commit 2bf36bb into dev Dec 17, 2018
@delete-merged-branch delete-merged-branch bot deleted the mozilla-slugify branch December 17, 2018 06:51
@ghost ghost removed the in progress label Dec 17, 2018
@OttoWinter OttoWinter mentioned this pull request Dec 17, 2018
Closed
@emontnemery emontnemery mentioned this pull request Dec 21, 2018
Closed
dshokouhi pushed a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018
@pvizeli @dshokouhi
Use unicode slugify (home-assistant#19192)
4cb9a4b 
* Update __init__.py

* Update setup.py

* Update requirements_all.txt

* Update __init__.py

* Update __init__.py

* Update __init__.py

* Update __init__.py

* Update __init__.py

* remove `-`

* fix packages

* Update package_constraints.txt

* Update __init__.py

* Update package_constraints.txt

* Update requirements_all.txt

* Update setup.py

* Fix tests

* Fix line issue

* fix all test

* fix type

* Fix lint
@syssi
Copy link
Member

syssi commented Jan 4, 2019

This change did break a lot of unique_ids in my setup:

# before
"unique_id": "coordination158d0002a4f9f8"

# after
"unique_id": "coordination_158d0002a4f9f8"

It looks like a "-" character was removed in the past. Now it's mapped from "-" to "_".

@syssi syssi mentioned this pull request Jan 4, 2019
Closed
@balloob
Copy link
Member

balloob commented Jan 4, 2019

uhm, unique IDs should not get slugified. Maybe you want to add a PR that copies the old slugify function into the xiaomi_aqara component and use that to prevent a breaking change.

@syssi
Copy link
Member

syssi commented Jan 4, 2019

I will update the xiaomi_aqara component.

@syssi syssi mentioned this pull request Jan 4, 2019
Merged
@syssi
Copy link
Member

syssi commented Jan 6, 2019

The PR will break all entity ids (without unique_id) which are containing a minus sign "-". In the past this character was removed. In future it is mapped to an underscore.

@balloob balloob mentioned this pull request Jan 10, 2019
Merged
@klaasnicolaas klaasnicolaas mentioned this pull request Jan 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@balloob balloob balloob approved these changes

Assignees

@pvizeli pvizeli

Labels
breaking-change cla-signed core small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pvizeli @amelchio @Anonym-tsk @balloob @syssi @houndci-bot @homeassistant