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

Allow context to be used like a dict. #54

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

gnat
Copy link
Contributor

@gnat gnat commented Jan 28, 2024

Allows context to optionally be used like a dict.

Nice because it exposes safe get() and makes the context syntax less opinionated.

Similar issue in Starlette: encode/starlette#2389

@nggit
Copy link
Owner

nggit commented Jan 30, 2024

That's a good idea to use a safe get().

BTW, the reason I used to create an opinionated set() function in dict is to be able to overwrite reserved properties like options and tasks consciously.

  1. maybe if __setitem__ is needed, it needs to raise ValueError, if it tries to overwrite reserved properties? then a slight modification in __setitem__ is needed.

  2. is __delitem__ really necessary? I always thought it is enough to set the member to None, instead of having to add the "don't delete reserved properties" logic as point 1.

@gnat
Copy link
Contributor Author

gnat commented Jan 30, 2024

  1. Agreed
  2. Agreed

Great suggestions. Updated the PR.

@nggit
Copy link
Owner

nggit commented Jan 30, 2024

I got an idea, maybe it's simpler with just a line of setattr:

    def __setitem__(self, *args):
        self.__setattr__(*args)

This will naturally raise an AttributeError if the user tries to overwrite the readonly property of either options or tasks. What do you think, @gnat?

@gnat
Copy link
Contributor Author

gnat commented Feb 1, 2024

Seems fine to me; tested it with a few different patterns and it didn't cause any issues. Updated!

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (40d9241) 92.34% compared to head (4503058) 92.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   92.34%   92.32%   -0.03%     
==========================================
  Files          19       19              
  Lines        1869     1875       +6     
==========================================
+ Hits         1726     1731       +5     
- Misses        143      144       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nggit
Copy link
Owner

nggit commented Feb 1, 2024

Thanks!

@nggit nggit merged commit 765d369 into nggit:master Feb 1, 2024
15 of 17 checks passed
@gnat
Copy link
Contributor Author

gnat commented Feb 2, 2024

Nice one.

nggit added a commit that referenced this pull request Aug 8, 2024
* Add dict style access to context.
* add test_servercontext

---------

Co-authored-by: nggit <[email protected]>
nggit added a commit that referenced this pull request Aug 8, 2024
* Add dict style access to context.
* add test_servercontext

---------

Co-authored-by: nggit <[email protected]>
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