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

Refactor mutation events registration by moving reusable code from relevant steps to EventUti #2609

Merged
merged 1 commit into from
May 24, 2024

Conversation

porunov
Copy link
Contributor

@porunov porunov commented May 17, 2024

This will allow to reuse callback registration logic for extended classes of mutation steps.

Related discussion where the conclusion was made to relax individual steps based on necessity: https://lists.apache.org/thread/vjbjh29kwjhd5lcmkqqqqrhw7rw2ynh9

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2024

Codecov Report

Attention: Patch coverage is 82.17822% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 76.81%. Comparing base (9b46b67) to head (a0e6869).
Report is 92 commits behind head on 3.7-dev.

Files Patch % Lines
...n/process/traversal/step/util/event/EventUtil.java 85.29% 4 Missing and 6 partials ⚠️
...ess/traversal/step/sideEffect/AddPropertyStep.java 65.21% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             3.7-dev    #2609      +/-   ##
=============================================
+ Coverage      76.14%   76.81%   +0.67%     
- Complexity     13152    13194      +42     
=============================================
  Files           1084     1087       +3     
  Lines          65160    66303    +1143     
  Branches        7285     7293       +8     
=============================================
+ Hits           49616    50931    +1315     
+ Misses         12839    12652     -187     
- Partials        2705     2720      +15     

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

@spmallette
Copy link
Contributor

do you think there should be utilities for all of these sorts of things? like this similar sort of code in AddVertexStep.

@porunov
Copy link
Contributor Author

porunov commented May 17, 2024

do you think there should be utilities for all of these sorts of things? like this similar sort of code in AddVertexStep.

I think the more we can make logic reusable the better. So, I think we can make utilities for all the relevant places where such event registration is used.

@xiazcy
Copy link
Contributor

xiazcy commented May 21, 2024

I'm good with the change, but I have similar question as Stephen. Is it worth adding one extra util file just for the removed events registration? I don't quite see these method used elsewhere.

For example, when refactoring for other types of event registration, it might be too much to AddedEventUtil or ChangedEventUtil. Perhaps renaming RemovedEventUtil to something more general, like EventUtil, so we can group refactored methods together for all event types?

@porunov
Copy link
Contributor Author

porunov commented May 23, 2024

I'm good with the change, but I have similar question as Stephen. Is it worth adding one extra util file just for the removed events registration? I don't quite see these method used elsewhere.

For example, when refactoring for other types of event registration, it might be too much to AddedEventUtil or ChangedEventUtil. Perhaps renaming RemovedEventUtil to something more general, like EventUtil, so we can group refactored methods together for all event types?

Definitely makes sense 👍 I will update it tomorrow.

…levant steps to EventUti

Related discussion where the conclusion was made to relax individual steps based on necessity: https://lists.apache.org/thread/vjbjh29kwjhd5lcmkqqqqrhw7rw2ynh9

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov porunov force-pushed the feature/refactor-drop-step branch from 0b115d9 to a0e6869 Compare May 23, 2024 12:30
@porunov porunov changed the title Refactor DropStep for extensibility Refactor mutation events registration by moving reusable code from relevant steps to EventUti May 23, 2024
@porunov
Copy link
Contributor Author

porunov commented May 23, 2024

@spmallette @xiazcy Pushed an update to make this PR more general. Refactored all places with events registration logic by moving reusable code to EventUtil.

@spmallette spmallette merged commit a0e6869 into apache:3.7-dev May 24, 2024
23 of 24 checks passed
@spmallette
Copy link
Contributor

thanks - added in a couple follow-on bits for formatting/javadoc: bd5836a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants