Skip to content

Migrate to new Plugin API #137

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 25 commits into from
Oct 25, 2024
Merged

Conversation

alexander-yevsyukov
Copy link
Collaborator

@alexander-yevsyukov alexander-yevsyukov commented Oct 10, 2024

This PR migrates Validation plugins to new Plugin API recently introduced in ProtoData.

Convenience extension functions asA() and asB() were also adopted.

@alexander-yevsyukov alexander-yevsyukov self-assigned this Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 18.66667% with 61 lines in your changes missing coverage. Please review.

Project coverage is 38.08%. Comparing base (77044b1) to head (acb12d4).
Report is 26 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #137      +/-   ##
============================================
+ Coverage     37.91%   38.08%   +0.16%     
- Complexity      353      362       +9     
============================================
  Files           125      125              
  Lines          2363     2450      +87     
  Branches        195      195              
============================================
+ Hits            896      933      +37     
- Misses         1405     1455      +50     
  Partials         62       62              

@alexander-yevsyukov alexander-yevsyukov changed the title Support field references in min_value and max_value Migrate to new Plugin API Oct 24, 2024
@alexander-yevsyukov alexander-yevsyukov marked this pull request as ready for review October 24, 2024 21:35
Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@alexander-yevsyukov please see my comments, and let's discuss.

@@ -59,7 +59,7 @@ protected EitherOf2<RuleAdded, Nothing> whenever(@External FieldExited event) {
var declaration = findField(event.getField(), event.getType(), event.getFile(), this);
return EitherOf2.withA(requiredRule(declaration, field));
}
return noReaction();
return ignoring();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep noReaction() here, as it reads better to me.

@@ -66,7 +66,7 @@ protected EitherOf2<RuleAdded, Nothing> whenever(@External FieldExited event) {
}
var shouldValidate = field != null && field.getValidate();
if (!shouldValidate) {
return noReaction();
return ignoring();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

protected final EitherOf2<RuleAdded, Nothing> noReaction() {
//TODO:2024-08-11:alexander.yevsyukov: Use EventProducer.noReaction() extension from `core-java`.
return EitherOf2.withB(nothing());
protected final EitherOf2<RuleAdded, NoReaction> ignoring() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comments above. And let's discuss.

// We have the option defined in the type. But is it set to `true`?
val option = event.option.value.unpack<BoolValue>()
if (!option.value) {
return EitherOf2.withB(nothing())
return noReaction().asB()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This piece isn't updated, while other similar expressions are.

Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii left a comment

Choose a reason for hiding this comment

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

LGTM from my side.

I would also go with noReaction() because it matches the method signature of the reactor.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@alexander-yevsyukov LGTM.

@yevhenii-nadtochii FYI. We have settled on ignore(), as noReaction() is impossible to have to return Either... instance — in the same class hierarchy it is already busy returning plain NoReaction. So there's that.

@alexander-yevsyukov alexander-yevsyukov merged commit 6093598 into master Oct 25, 2024
7 checks passed
@alexander-yevsyukov alexander-yevsyukov deleted the bump-mc-java-and-protodata branch October 25, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants