Skip to content

Issue 7290: removed duplicate tableProperties instance - #7678

Merged
patchwork01 merged 2 commits into
gchq:developfrom
TheOnlyOrchid:develop
Jul 20, 2026
Merged

Issue 7290: removed duplicate tableProperties instance#7678
patchwork01 merged 2 commits into
gchq:developfrom
TheOnlyOrchid:develop

Conversation

@TheOnlyOrchid

Copy link
Copy Markdown
Contributor

Issue

Tests

  • My PR does not add any tests, as it is a small PR to edit a singular test

Documentation

  • My PR does not add any documentation, as it is a small change that, in my opinion, does not need any documentation.
    • If anyone disagrees with this assertion, I am happy to review and add any necessary documentation.


// When
List<Row> filtered = applyIterator(rows, tablePropertiesNullableValueField);
List<Row> filtered = applyIterator(rows, tableProperties);

@patchwork01 patchwork01 Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks very much for the help. Now that there's only one TableProperties field we don't need it to be a parameter for the helper methods any more. Do you want to update applyIterator and createIterator as part of this PR, or should we make that a separate issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would be happy to have a look at that later today! I will hopefully have a pull request posted
by tomorrow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added another commit that changes the function signature of applyIterator and createIterator to remove the redundant parameters.

I also noticed that with the new changes, it may be worth changing the function names, so it is easier to interpret its behaviour.

Previously you could infer the intent was to create an iterator for the tableProperties field, as the function call would include "tableProperties". However the function signature change now obscures this behaviour, requiring manual inspection of the function to interpret the behaviour.

Unfortunately I am now unavailable for the next two weeks, so to streamline the process I did not include this change, as it may require further input from me.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see what you mean. I'd be happy with that, or if you preferred to leave applyIterator and createIterator as they were. I think we can go ahead with what we've got, thanks.

…and AggregationFilteringIteratorTest.applyIterator to remove redundant parameters
@patchwork01 patchwork01 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 17, 2026
@patchwork01
patchwork01 merged commit ee8c424 into gchq:develop Jul 20, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-reviewer Pull requests that need a reviewer to be assigned

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove extra TableProperties from AggregationFilteringIteratorTest

3 participants