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

Update experiment API #1065

Merged
merged 4 commits into from
Mar 11, 2025
Merged

Update experiment API #1065

merged 4 commits into from
Mar 11, 2025

Conversation

KochTobi
Copy link
Member

No description provided.

@KochTobi KochTobi requested a review from a team as a code owner March 11, 2025 07:09
Steffengreiner
Steffengreiner previously approved these changes Mar 11, 2025
Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

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

Heya @KochTobi, I tested it out and it works, nicely done! However i have a question and minor request for updated JD which would be nice if they were addressed

Comment on lines +247 to 251
sealed interface ExperimentUpdateRequestBody permits ConfoundingVariableAdditions,
ConfoundingVariableDeletions, ConfoundingVariableUpdates, ExperimentDescription,
ExperimentalGroups, ExperimentalVariableAdditions, ExperimentalVariableDeletions {

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since i'm not up to date i assume that i missed something so here's my question:

What's the advantage of having an interface that could be any of these possibilities?
As i saw below, this leads to the burden of the service implementation to account for all cases individually.
Could this be seperated into individual use cases to avoid an extending switch statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

The advantage is that we do not have a lot of interface methods while the switch enforces us to implement each possible input that can be there. Given that the switch statement does not have any default path it is clear what is done where and we do not need to copy shared code (like the request id and so on) to all the single methods. Modifying a retry strategy also can be handled centrally if desired.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be separated into different methods which would make it easier to specify what should be done with the input. Deciding based on what was sent works too.

Co-authored-by: steffengreiner <[email protected]>
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@KochTobi
Copy link
Member Author

Heya @KochTobi, I tested it out and it works, nicely done! However i have a question and minor request for updated JD which would be nice if they were addressed

Hi Steffen, the changes I introduced you cannot really test out. This PR only adds the interface methods for changing the experimental variables, experimental groups and the confounding variables of experiments. It does not offer any implementation.

@KochTobi KochTobi requested a review from a team March 11, 2025 13:52
Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, makes sense to me.
To clarify i tested if any of the current processes break which doesn't happen 👍

@KochTobi KochTobi merged commit afe820f into development Mar 11, 2025
5 of 6 checks passed
@KochTobi KochTobi deleted the feature/api-for-variables branch March 11, 2025 13: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.

2 participants