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

feat: Change fractional custom op from percentage-based to relative weighting. #91

Conversation

aepfli
Copy link
Member

@aepfli aepfli commented Jun 19, 2024

…eighting.

Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli requested a review from a team as a code owner June 19, 2024 17:26
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.45%. Comparing base (d8e10c7) to head (992b7b9).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   90.55%   92.45%   +1.90%     
==========================================
  Files           8       12       +4     
  Lines         180      464     +284     
==========================================
+ Hits          163      429     +266     
- Misses         17       35      +18     
Flag Coverage Δ
unittests ?

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

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

Copy link
Member

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

I've added a few styling related comments, but this PR will also need additional test coverage.

I also find the logic quite difficult to follow. Can we try to avoid multiple nested if statements? And can we perhaps make the error logs more descriptive about why they are error cases?

@aepfli
Copy link
Member Author

aepfli commented Jun 20, 2024

I've added a few styling related comments, but this PR will also need additional test coverage.

I also find the logic quite difficult to follow. Can we try to avoid multiple nested if statements? And can we perhaps make the error logs more descriptive about why they are error cases?

I am working on adding further tests to the test harness. But i can also try to add here more tests. python, as you might have guessed, is not my strong suit. but i will try to adapt

@aepfli
Copy link
Member Author

aepfli commented Jun 20, 2024

the python sdk only logs and error and continues its targeting evaluation - compared to the other sdks, eg java or js - is this a unintended difference between those, should we also unify this, and instead of logging an error raise an exception?

Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the issue/fractional_relative_weighting branch from 67b69cb to 05a8cca Compare June 20, 2024 11:39
@Kavindu-Dodan
Copy link

Kavindu-Dodan commented Jun 21, 2024

the python sdk only logs and error and continues its targeting evaluation - compared to the other sdks, eg java or js - is this a unintended difference between those, should we also unify this, and instead of logging an error raise an exception?

+1 for this proposal. I had the same doubt when reviewing the PR. To be consistent, yes we should raise exceptions

Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice work, added just some minor comments

aepfli and others added 2 commits June 26, 2024 08:19

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…rovider/flagd/resolvers/process/custom_ops.py

Co-authored-by: Anton Grübel <anton.gruebel@gmail.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
…rovider/flagd/resolvers/process/custom_ops.py

Co-authored-by: Anton Grübel <anton.gruebel@gmail.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the issue/fractional_relative_weighting branch from 48e44fd to c0f4a55 Compare June 26, 2024 11:00
Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

thanks, looks good 🍻

@gruebel
Copy link
Member

gruebel commented Jun 27, 2024

@beeme1mr do you have any idea about the failing uploads to codecov? I thought we fixed that issue or is it something new?

@beeme1mr
Copy link
Member

@beeme1mr do you have any idea about the failing uploads to codecov? I thought we fixed that issue or is it something new?

I'm not sure what's wrong. The logs indicate that the codecov secret isn't set but the configuration looks correct and it was working last week.

…rovider/flagd/resolvers/process/custom_ops.py

Co-authored-by: Matthew Elwell <mjelwell89@gmail.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
@aepfli aepfli force-pushed the issue/fractional_relative_weighting branch from 460be94 to 06e7a5c Compare July 8, 2024 19:12
@aepfli
Copy link
Member Author

aepfli commented Jul 9, 2024

Any help for codecov is highly appreciated - it seems that this is the only reason the builds are failing

@beeme1mr
Copy link
Member

@aepfli could you please rebase? I think I've resolved the CodeCov issue with this PR.

@aepfli
Copy link
Member Author

aepfli commented Jul 23, 2024

done i am not sure if anything else is missing here ;) but please let me know

@beeme1mr beeme1mr merged commit 7b34822 into open-feature:main Jul 23, 2024
17 checks passed
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.

None yet

5 participants