Skip to content

fix: Fix type checking #25

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

Merged
merged 6 commits into from
Nov 21, 2022
Merged

Conversation

mschoenlaub
Copy link
Contributor

@mschoenlaub mschoenlaub commented Oct 17, 2022

Signed-off-by: Manuel Schönlaub [email protected]

This PR

This PR fixes typing errors in the client code:

  1. The Liskov Substitution Principle should also apply to providers. This relates to the signature of the methods to retrieve flags.

  2. Incorrect usage of type instead of flagType. Most probably a typo.

  3. Ensure that the callables in create_provider_evaluation are annotated in a mypy-friendly way.

  4. Fix type annotations for FlagEvaluationDetails

  5. Fix type annotations around optional parameters all over the place

Related Issues

Fixes #17

@mschoenlaub mschoenlaub changed the title WIP: Fix type checking within client fix: Fix type checking within client Oct 17, 2022
@mschoenlaub mschoenlaub marked this pull request as ready for review October 17, 2022 18:01
@beeme1mr beeme1mr requested review from ajhelsby, matthewelwell and dlopes7 and removed request for ajhelsby October 18, 2022 02:24
@mschoenlaub mschoenlaub force-pushed the fix/error-message branch 2 times, most recently from 73cc649 to 593896c Compare October 19, 2022 21:55
@mschoenlaub mschoenlaub changed the title fix: Fix type checking within client fix: Fix type checking within client and HookSupport Oct 19, 2022
Copy link
Member

@tcarrio tcarrio left a comment

Choose a reason for hiding this comment

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

Solid improvements to the typing and good catch on the type vs flag_type reference 👌

Signed-off-by: Manuel Schönlaub <[email protected]>
Signed-off-by: Manuel Schönlaub <[email protected]>
Signed-off-by: Manuel Schönlaub <[email protected]>
Signed-off-by: Manuel Schönlaub <[email protected]>
@mschoenlaub mschoenlaub force-pushed the fix/error-message branch 2 times, most recently from 9b085a5 to e5cebb6 Compare November 12, 2022 02:10
Signed-off-by: Manuel Schönlaub <[email protected]>
@mschoenlaub mschoenlaub requested review from matthewelwell and tcarrio and removed request for ajhelsby, dlopes7, matthewelwell and tcarrio November 12, 2022 02:13
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2022

Codecov Report

Merging #25 (7102813) into main (04a4323) will increase coverage by 0.08%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   92.83%   92.92%   +0.08%     
==========================================
  Files          18       18              
  Lines         321      325       +4     
==========================================
+ Hits          298      302       +4     
  Misses         23       23              
Flag Coverage Δ
unittests 92.92% <78.26%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
open_feature/hooks/hook_support.py 95.00% <ø> (ø)
open_feature/provider/no_op_provider.py 100.00% <ø> (ø)
open_feature/exception/exceptions.py 79.16% <37.50%> (ø)
...n_feature/evaluation_context/evaluation_context.py 90.90% <100.00%> (+0.90%) ⬆️
...feature/flag_evaluation/flag_evaluation_details.py 100.00% <100.00%> (ø)
open_feature/hooks/hook_context.py 100.00% <100.00%> (ø)
open_feature/open_feature_api.py 100.00% <100.00%> (ø)
open_feature/open_feature_client.py 96.29% <100.00%> (+0.04%) ⬆️
open_feature/provider/provider.py 75.00% <100.00%> (+0.92%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mschoenlaub
Copy link
Contributor Author

mschoenlaub commented Nov 12, 2022

I reworked the whole thing as plenty of conflicts had come up in the meantime.
I also extended the scope of the PR in an effort to avoid revisiting the annotation topic again.

In particular I unified now that name and version for a client are optional (the get_client hat a different opinion of that than the actual client class), made sure there was something in place to avoid arbitrary functions to be added to a mapping from FlagType to the respective callables and made it possible that error message is now optional, but error code is not. That required changing the order of arguments there though.

@mschoenlaub mschoenlaub changed the title fix: Fix type checking within client and HookSupport fix: Fix type checking Nov 12, 2022
@mschoenlaub mschoenlaub requested review from tcarrio and matthewelwell and removed request for matthewelwell and tcarrio November 15, 2022 19:47
@beeme1mr beeme1mr requested a review from toddbaert November 21, 2022 18:11
@toddbaert toddbaert self-requested a review November 21, 2022 18:32
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

I don't have much Python experience, but all the type hints here look correct to me. Thanks a lot for this contribution. Much appreciated.

@toddbaert
Copy link
Member

@mschoenlaub I've invited you to the python maintainers group. Feel free to accept or not. Soon we will be formalizing the responsibility of that role (you can leave it anytime you want, of course) but it will allow you to merge for now if you'd like.

Copy link
Member

@tcarrio tcarrio left a comment

Choose a reason for hiding this comment

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

Same request as @toddbaert, in regards to making the provider required and defaulting to a no-op provider initially.

@tcarrio
Copy link
Member

tcarrio commented Nov 21, 2022

We could also instead just merge this as typing enhancements and resolve the optional provider issue separately. In the name of progress 🚀

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.

[BUG] ExceptionObject has no attribute error_message
5 participants