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.

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

PEP 681: Remove hash alias for unsafe_hash #2454

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Conversation

debonte
Copy link
Contributor

@debonte debonte commented Mar 21, 2022

@davidfstr pointed out that having a hash alias for unsafe_hash is disguising a potentially unsafe feature as safe.

We don't remember why we added a hash alias. It may have been a mistake. Or it may have been added because attrs uses hash instead of unsafe_hash. However, the attrs documentation recommends against setting hash, so this should be unusual. Attrs can add an unsafe_hash alias on their side if needed.

cc: @hynek

@hynek
Copy link
Member

hynek commented Mar 23, 2022

I understand that the field argument is not relevant to this PEP, but it seems like this PEP is kind of enforcing to have a inconsistent naming between decorator (unsafe_hash) and field (hash)?

@CAM-Gerlach
Copy link
Member

@hynek You might want to bring this up on the PEP's discussion thread

@debonte
Copy link
Contributor Author

debonte commented Mar 23, 2022

inconsistent naming between decorator (unsafe_hash) and field (hash)?

@hynek, I don't see this as an inconsistency. The hash field parameter determines whether the field will be included in hash calculations if a __hash__ method is generated. A __hash__ method can be generated without explicitly setting unsafe_hash, for example when "eq and frozen are both True" (quoted from PEP 557). The word "unsafe" in the name of unsafe_hash is a warning that setting the value explicitly is not recommended. However, setting the hash property on fields is safe.

@hynek
Copy link
Member

hynek commented Mar 23, 2022

The was my first thought too, so I checked the dataclasses docs and it says:

hash: This can be a bool or None. If true, this field is included in the generated hash() method. If None (the default), use the value of compare: this would normally be the expected behavior. A field should be considered in the hash if it’s used for comparisons. Setting this value to anything other than None is discouraged.

The discouraged part makes it weird to me to have one argument have a different name than the overarching one.

Anyhow it’s not that I super care about it, it just sprung into my eye while looking at related attrs code. I’m currently sick though on a phone, so I have no time to work on this in any way (including PEP threads).

I try to add the alias for the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants