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

Intro'd freeze/unfreeze. #130

Merged
merged 2 commits into from
Nov 19, 2020
Merged

Intro'd freeze/unfreeze. #130

merged 2 commits into from
Nov 19, 2020

Conversation

sumukhbarve
Copy link
Contributor

Based on my reading of #121, it's clear that the need for .freeze() arises from the desire for KeyErrors, so that typos can be caught quickly. Keeping this in mind, I've tried to make minimal additions.

The Basics

When an addict.Dict is frozen, accessing a missing key will raise a KeyError. This is true for nested addict.Dict instances too.

Allowing Key Addition

In some sense, a plain Python dict is already frozen, as it always raises KeyErrors. Note that plain dicts allow the addition of new keys. Thus, it should make sense to allow the addition to new keys to frozen addicts, but only at the top level. That is:

  • frozenDicty.newKey = 1 should work as expected, but
  • frozenDicty.missingKey.newKey = 1 should raise a KeyError.

Unfreezing is a Shorthand

As .freeze() and .unfreeze() are very similar, differing only in a boolean flag, it made sense to implement .unfreeze() as a shorthand for .freeze(False). Specifically:

  • .freeze() is equivalent to .freeze(True), and
  • .unfreeze() is a shorthand for .freeze(False).

No Frozen Initialization

Thought about adding a __frozen param to __init__(), but decided against it. If required, such functionality can easily be added later. And while a such a parameter is mentioned in #121, in favoring explicit vs implicit, it may be best to require the user to explicitly call .freeze().

No Return Value

Currently, .freeze() (including .unfreeze()) returns None, not an addict.Dict. This is similar to Python's built-in dict.update(). While #121 suggests making .freeze() return a frozen addict.Dict, retaining parity with dict.update() may be desirable.

@coveralls
Copy link

coveralls commented Nov 18, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 227d667 on polydojo:master into 01b8f76 on mewwts:master.

@mewwts
Copy link
Owner

mewwts commented Nov 19, 2020

Thank you for your contribution and very clear PR @sumukhbarve!

I agree with most of your changes, but in my opinion we should throw KeyError on all new keys for consistency. Being "frozen" implies immutability, not compatibility with the regular dict. For compatibility we have to_dict().

What do you think?

@sumukhbarve
Copy link
Contributor Author

sumukhbarve commented Nov 19, 2020

Hi Matts,

On second thought, I agree with you. Frozen-ness needn't be the same a dict-compatibility.

I've made the required changes. Frozen Dicts now throw KeyErrors on setting new keys.

@mewwts
Copy link
Owner

mewwts commented Nov 19, 2020

Great thank you! Would you mind rebasing this on master so that we can run tests here on github actions?

@sumukhbarve
Copy link
Contributor Author

Done!

@mewwts mewwts merged commit 7e8d23d into mewwts:master Nov 19, 2020
@mewwts
Copy link
Owner

mewwts commented Nov 19, 2020

Thanks!

@mewwts
Copy link
Owner

mewwts commented Nov 19, 2020

Could you draft up a simple release note for this @sumukhbarve?

@sumukhbarve
Copy link
Contributor Author

Hi @mewwts,

Thank you for merging the PR! :)

With regard to release notes, how about:

Title: Introduced freeze/unfreeze.
Description: Calling .freeze() forbids getting/setting missing keys. Use .unfreeze() to undo. Ref #121, #130.

Cheers,
Sumukh

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.

3 participants