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

gh-2425 remove duplicate updateOperationInput #2426

Merged
merged 6 commits into from
Aug 27, 2021

Conversation

GCHQDev404
Copy link
Contributor

@GCHQDev404 GCHQDev404 commented May 17, 2021

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2021

Codecov Report

Merging #2426 (9ff4732) into develop (cae6d6b) will decrease coverage by 0.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2426      +/-   ##
=============================================
- Coverage      66.87%   66.39%   -0.49%     
+ Complexity      2439     2437       -2     
=============================================
  Files            967      932      -35     
  Lines          31749    30242    -1507     
  Branches        3857     3645     -212     
=============================================
- Hits           21232    20078    -1154     
+ Misses          8863     8588     -275     
+ Partials        1654     1576      -78     
Impacted Files Coverage Δ
...er/access/predicate/user/DefaultUserPredicate.java 57.14% <ø> (ø)
...gchq/gaffer/graph/hook/NamedOperationResolver.java 85.18% <ø> (+1.31%) ⬆️
...store/operation/handler/OperationChainHandler.java 91.30% <ø> (+18.33%) ⬆️
.../java/uk/gov/gchq/gaffer/spark/SparkConstants.java 50.00% <ø> (ø)
...v/gchq/gaffer/spark/utils/scala/DataFrameUtil.java 0.00% <ø> (ø)
...hq/gaffer/rest/controller/OperationController.java 76.05% <ø> (ø)
...e/operation/handler/util/OperationHandlerUtil.java 75.00% <100.00%> (+20.00%) ⬆️
.../gov/gchq/gaffer/data/graph/entity/EntityMaps.java 71.42% <0.00%> (-14.29%) ⬇️
...chq/gaffer/data/graph/adjacency/AdjacencyMaps.java 85.71% <0.00%> (-14.29%) ⬇️
.../java/uk/gov/gchq/gaffer/commonutil/OneOrMore.java 74.32% <0.00%> (-2.71%) ⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23c323e...9ff4732. Read the comment docs.

@n3101
Copy link

n3101 commented May 18, 2021

@GCHQDev404 Is this a breaking change?

@GCHQDev404
Copy link
Contributor Author

GCHQDev404 commented Jun 3, 2021

@n3101

I wouldn't consider it a breaking change, you need someone in the wild extending the class and overriding the fundamental protected inputUpdate. You might as well implement your own Handler or override the doOperation.

Would you prefer me to add back in protected method and a junk call out to util?

 protected void updateOperationInput(final Operation op, final Object result) {
        OperationHandlerUtil.updateOperationInput(op, result);
    }

@GCHQDev404 GCHQDev404 added the question Specific query about part of the codebase label Jun 3, 2021
@n3101
Copy link

n3101 commented Jun 9, 2021

@GCHQDev404 I refer your question to @d21211122 to answer

@GCHQDev404
Copy link
Contributor Author

GCHQDev404 commented Jun 15, 2021

@n3101

I wouldn't consider it a breaking change, you need someone in the wild extending the class and overriding the fundamental protected inputUpdate. You might as well implement your own Handler or override the doOperation.

Would you prefer me to add back in protected method and a junk call out to util?

 protected void updateOperationInput(final Operation op, final Object result) {
        OperationHandlerUtil.updateOperationInput(op, result);
    }

I'm going to self answer no. because it would be a dead hanging method. because I would call the util method. If someone wants to override how to update the input of operations in a chain then can self implement the method.

@GCHQDev404 GCHQDev404 removed the question Specific query about part of the codebase label Jun 15, 2021
Copy link
Contributor

@t92549 t92549 left a comment

Choose a reason for hiding this comment

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

I agree this isn't a breaking change unless someone is doing something very strange, in which case it will be an easy fix anyway

@t92549 t92549 merged commit c18c39d into develop Aug 27, 2021
@t92549 t92549 deleted the gh-2425-remove-duplicate-upgradeOperationInput branch August 27, 2021 08:59
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.

Remove duplicate updateOperationInput
4 participants