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: remove the deprecated setTargetingKey method in EvaluationContext. #290

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

thiyagu06
Copy link
Member

Signed-off-by: thiyagu06 [email protected]

This PR

remove the deprecated setTargetingKey method in EvaluationContext.

Related Issues

#253

Notes

Follow-up Tasks

How to test

@thiyagu06 thiyagu06 changed the title remove the deprecated setTargetingKey method in EvaluationContext. feat: remove the deprecated setTargetingKey method in EvaluationContext. Feb 10, 2023
@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #290 (a8139cc) into main (4102495) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #290      +/-   ##
============================================
- Coverage     91.82%   91.80%   -0.02%     
+ Complexity      213      212       -1     
============================================
  Files            23       23              
  Lines           489      488       -1     
  Branches         30       30              
============================================
- Hits            449      448       -1     
  Misses           24       24              
  Partials         16       16              
Flag Coverage Δ
unittests 91.80% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...ain/java/dev/openfeature/sdk/ImmutableContext.java 90.47% <ø> (-0.44%) ⬇️

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

@thiyagu06
Copy link
Member Author

  1. I still kept the setTargetingKey in the mutableContext Implementation. I feel it's appropriate for mutableContext to allow the mutation of targetingKey. Any thoughts?
  2. Confirm that we're not going to do major release for this deprecation.

@toddbaert toddbaert changed the title feat: remove the deprecated setTargetingKey method in EvaluationContext. feat!: remove the deprecated setTargetingKey method in EvaluationContext. Feb 10, 2023
@toddbaert toddbaert marked this pull request as draft February 10, 2023 15:01
@toddbaert toddbaert changed the title feat!: remove the deprecated setTargetingKey method in EvaluationContext. feat: remove the deprecated setTargetingKey method in EvaluationContext. Feb 10, 2023
@toddbaert
Copy link
Member

Thanks @thiyagu06 . I think regardless of whether this is a major version increase or not, we need to wait a while before we release it. The current deprecation warning should be there for at least a month or so to alert consumers.

@justinabrahms
Copy link
Member

@toddbaert Is it not adequate in your mind to just rely on version numbers for this?

@toddbaert
Copy link
Member

toddbaert commented Feb 10, 2023

@justinabrahms

@toddbaert Is it not adequate in your mind to just rely on version numbers for this?

We can! But I think technically, unless I'm missing something, removing this from the interface is a breaking change, so if we're being very strict it would mean this has to be a 2.0 release (we'd have to change the title to feat! ... to denote that, and release please would do the rest).

However, as per the spec, evaluation context is still experimental, and thus is subject to breaking changes on a minor release, so we have some leeway here.

It's such a small change, I don't mind either way, but I wanted to make sure we were all aware of this. I specifically wanted to get your opinion.

@justinabrahms
Copy link
Member

I'm fine with breaking this on a minor change given the experimental status.

@toddbaert toddbaert marked this pull request as ready for review February 10, 2023 20:27
@toddbaert
Copy link
Member

@justinabrahms cool! I'll merge then and we can release this in a bit once the deprecation warning has floated around for a while (that was released today).

@toddbaert toddbaert force-pushed the remove-deprected-method branch from 9622ee9 to a8139cc Compare February 10, 2023 20:28
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@toddbaert toddbaert merged commit d78c99c into open-feature:main Feb 10, 2023
@thiyagu06 thiyagu06 deleted the remove-deprected-method branch February 11, 2023 05:35
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