-
Notifications
You must be signed in to change notification settings - Fork 58
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
Modified backend and frontend for handling grouping of datasets #460
base: master
Are you sure you want to change the base?
Modified backend and frontend for handling grouping of datasets #460
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing "Ungrouped" or "Others" everywhere is not a good way to handle such a problem. Since the group is optional, the classes should offer a constructor without this attribute. The default value (e.g., "Others") should be provided in a static final class attribute.
There are also some other smaller changes needed. Please have a look at the single comments.
src/main/java/org/aksw/gerbil/annotator/AnnotatorConfigurationImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/aksw/gerbil/dataset/InstanceListBasedDataset.java
Outdated
Show resolved
Hide resolved
src/main/java/org/aksw/gerbil/dataset/InstanceListBasedDataset.java
Outdated
Show resolved
Hide resolved
src/main/java/org/aksw/gerbil/dataset/datahub/DatahubNIFConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/org/aksw/gerbil/dataset/impl/nif/NIFFileDatasetConfig.java
Outdated
Show resolved
Hide resolved
src/test/java/org/aksw/gerbil/annotator/TestAnnotatorConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/org/aksw/gerbil/annotator/decorator/ErrorCountingAnnotatorDecoratorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/aksw/gerbil/annotator/decorator/ErrorCountingAnnotatorDecoratorTest.java
Outdated
Show resolved
Hide resolved
I have read your comments on the code and would refactor them as per your comments @MichaelRoeder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is just a small change request to make things nicer 🙂
@@ -306,18 +302,20 @@ public ModelAndView experiment(@RequestParam(value = "id") String id, HttpServle | |||
} | |||
|
|||
@RequestMapping("/datasets") | |||
public @ResponseBody List<String> datasets(@RequestParam(value = "experimentType") String experimentType) { | |||
public @ResponseBody Map<String, List<String>> datasets(@RequestParam(value = "experimentType") String experimentType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a solution that would work 👍
However, the current implementation handles something that the Spring framework can already do for you. I am afraid that I have described the goal in an ambiguous way, so let me try to explain what I would suggest.
The method should return Map<String, List<DatasetCofiguration>>
and the Spring framework should apply it's "magic" to transform it into JSON 🪄
Of course, Spring cannot do magic things, so we still have to provide the serialization 😉
However, it is sufficient if we tell Spring that there is exactly one class that implements the serialization and Spring can simply make use of it whenever it is necessary. You can do that with the method that is shown under point 3.2 in https://www.baeldung.com/spring-boot-customize-jackson-objectmapper (if I am not mistaken, I didn't read the whole text 😅 so let me know if you encounter problems). This method should go into our RootConfig
.
src/test/java/org/aksw/gerbil/annotator/decorator/ErrorCountingAnnotatorDecoratorTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UI looks good but I think that we have a general misunderstanding how the workflow to achieve the targeted functionality should look like 😅
So let me bring up my points why I think the current solution is not exactly what it should be...
MainController
: I do not get, why the controller returns aMap<String, ...>
. I assume that is done to make it easier for the UI to show the groups, because the data already comes in the form that the UI would like to have it. However, this is actually not really the task of the controller. If the UI would like to have the data in a certain form, it is free to organize the datasets by groups by itself. It is not really the task of the server to do that especially if we keep in mind that other (future) clients of the API are not interested in the group-wise order of datasets.MainController
: the serialization is still explicitly done by ourselves while it should be done by the Spring framework. Yes, we can implement it by ourselves but what should we return in case of an error? At the moment,null
is returned but that does not fit to the HTTP standard. So why shouldn't we let Spring do that for us, since it has already implemented all the error handling, etc...
I also added a comment that shows how I imagine it to look like.
src/test/java/org/aksw/gerbil/annotator/decorator/ErrorCountingAnnotatorDecoratorTest.java
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a small comment. Otherwise, it looks good to me.
|
||
|
||
public AdapterConfigSerializer(Class<AbstractAdapterConfiguration> t) { | ||
super(AbstractAdapterConfiguration.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be super(t);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also what the Codacy check marked
https://app.codacy.com/gh/dice-group/gerbil/pull-requests/460/issues#issue-dd4f822d621fb37fa185c60c364e8db0
No description provided.