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

MCR-3198 event handler for merging duplicate categories #2267

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

erodde
Copy link
Contributor

@erodde erodde commented Sep 18, 2024

@erodde erodde requested review from toKrause and kkrebs September 24, 2024 14:52
@erodde erodde marked this pull request as ready for review September 24, 2024 14:52
@erodde
Copy link
Contributor Author

erodde commented Oct 9, 2024

In the video conference yesterday the question came up whether to include the new code in the MCRClassificationMappingEventHandler or leave it in its own EventHandler.

rsteph-de
rsteph-de previously approved these changes Oct 24, 2024
rsteph-de
rsteph-de previously approved these changes Oct 24, 2024
Copy link
Member

@rsteph-de rsteph-de left a comment

Choose a reason for hiding this comment

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

perfect!

Copy link
Contributor

@toKrause toKrause left a comment

Choose a reason for hiding this comment

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

I have created further test cases, not all of which work and for which I am not necessarily sure what the expected result actually is. The description of the issue doesn't provide enough information. You can merge this PR into this PR to obtain these test cases.

But as it is, I think too many cases are not (correctly) taken into account. Maybe the issue (and then the event handler implementation) needs to be reduced in scope in order to to get rid of these problems.

This probably needs to be discussed again in the next web conference.

@erodde erodde marked this pull request as draft December 4, 2024 15:35
@erodde erodde requested review from toKrause and golsch December 6, 2024 07:45
@erodde erodde marked this pull request as ready for review December 6, 2024 07:47
@erodde
Copy link
Contributor Author

erodde commented Dec 6, 2024

After our conversation on the 3rd of December I implemented the discussed changes. The Handlers are now ready for review.

@erodde erodde dismissed toKrause’s stale review December 6, 2024 13:33

We discussed open points and I refactored the code.

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.

5 participants