Skip to content

Refactor controllers to avoid boilerplate code#39

Merged
Elscrux merged 23 commits intodevelopfrom
refactor/controllers
Sep 8, 2023
Merged

Refactor controllers to avoid boilerplate code#39
Elscrux merged 23 commits intodevelopfrom
refactor/controllers

Conversation

@schweikart
Copy link
Copy Markdown
Member

This pull request replaces our controllers with a generic router implementation that discovers and routes new meta solvers (problem types) automatically.

@schweikart
Copy link
Copy Markdown
Member Author

schweikart commented Aug 1, 2023

Feature Model tests are currently failing because the feature model anomaly solver currently does not use the same route structure as the other solvers. We can solve this problem by separating all feature model anomalies into different problem types (implemented in #40 ) or by combining the feature model anomaly routes into a single route and adding an AnomalyType parameter to FeatureModelAnomalyRequest. We can discuss this at our meeting tomorrow

@Elscrux Elscrux self-requested a review August 2, 2023 09:27
@Elscrux
Copy link
Copy Markdown
Member

Elscrux commented Aug 6, 2023

Cross Origin is still a problem here, and we'll have to sort that out for the new webflux framework.

Copy link
Copy Markdown
Member

@Elscrux Elscrux left a comment

Choose a reason for hiding this comment

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

Looks good so far! The functional approach really shines here.
I think we could move the documentation part to its own method in each class, similarly to the handle method. That would separate this part a bit, reduce nesting and the method name would also help to document what this is for (I first didn't really get what this is about until you told me).

Comment thread src/main/java/edu/kit/provideq/toolbox/api/MetaSolverSettingsRouter.java Outdated
Comment thread src/main/java/edu/kit/provideq/toolbox/api/SolveRouter.java Outdated
Comment thread src/main/java/edu/kit/provideq/toolbox/api/SolversRouter.java Outdated
Comment thread src/main/java/edu/kit/provideq/toolbox/api/SubRoutineRouter.java Outdated
Comment thread src/main/java/edu/kit/provideq/toolbox/api/SolveRouter.java Outdated
@schweikart
Copy link
Copy Markdown
Member Author

Looks good so far! The functional approach really shines here. I think we could move the documentation part to its own method in each class, similarly to the handle method. That would separate this part a bit, reduce nesting and the method name would also help to document what this is for (I first didn't really get what this is about until you told me).

That seems like a good idea! Can you address this in your PR for API documentation?

@Elscrux
Copy link
Copy Markdown
Member

Elscrux commented Sep 3, 2023

Cross Origin is still a problem here, and we'll have to sort that out for the new webflux framework.

Tested again with the latest versions of the web (merged PR) and toolbox (anomaly refactor), this should have been fixed.
Edit: Oh I see you enabled CORS again.

@Elscrux
Copy link
Copy Markdown
Member

Elscrux commented Sep 3, 2023

Looks good so far! The functional approach really shines here. I think we could move the documentation part to its own method in each class, similarly to the handle method. That would separate this part a bit, reduce nesting and the method name would also help to document what this is for (I first didn't really get what this is about until you told me).

That seems like a good idea! Can you address this in your PR for API documentation?

Yeah I can do that as part of the API documentation.

Copy link
Copy Markdown
Member

@Elscrux Elscrux left a comment

Choose a reason for hiding this comment

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

This all looks fine to me now, but I'll wait for your SolutionHandle removal (if you want to do that, and do it in this PR)

@schweikart
Copy link
Copy Markdown
Member Author

This all looks fine to me now, but I'll wait for your SolutionHandle removal (if you want to do that, and do it in this PR)

I'll do that in another PR but please merge #40 before merging this one :)

@Elscrux Elscrux merged commit a018cb0 into develop Sep 8, 2023
@schweikart schweikart mentioned this pull request Sep 28, 2023
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