Restructure project #193
Restructure project #193
Conversation
- Create package for service classes and move them there - Adjust scopes for (service) functions to not to break current functionality
- Create package for repository classes and move them there
- Create package for configuration classes and move them there - Adjust class name to represent it's function
- Create package for controller classes and move them there
- Create package for util classes and move them there
- Extract endpoint functions into interfaces - Move endpoint (swagger) documentation into interfaces - Remove unused imports from controller classes
- Move classes into corresponding packages
- Rename controller classes - Rename controller interfaces
- Some classes had incorrect linting that has been fixed
imretoth-ess
requested review from
anderslindho,
domonkos-ess,
jacomago and
simon-ess
|
anderslindho
left a comment
anderslindho
left a comment
There was a problem hiding this comment.
I think this looks good, but I'll defer approval to the next one in line to review this.
jacomago
left a comment
jacomago
left a comment
There was a problem hiding this comment.
Couple of other things to do
| public class Application implements ApplicationRunner { | ||
|
|
||
| static final Logger logger = Logger.getLogger(Application.class.getName()); | ||
| public static final Logger logger = Logger.getLogger(Application.class.getName()); |
There was a problem hiding this comment.
Not your fault, but we shouldn't have public loggers. They should be all private. Think you can fix this one?
There was a problem hiding this comment.
Sure, I will take a look at that!
| @@ -1,4 +1,4 @@ | |||
| package org.phoebus.channelfinder.processors.aa; | |||
| package org.phoebus.channelfinder.service; | |||
There was a problem hiding this comment.
maybe make a sub external package for this class.
| @DeleteMapping("/{propertyName}/{channelName}") | ||
| @Override | ||
| public void removeSingle( | ||
| @PathVariable("propertyName") final String propertyName, |
There was a problem hiding this comment.
Should we remove the pathvariable annotations now they are in the interface?
There was a problem hiding this comment.
Good catch, I will do that!
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; |
There was a problem hiding this comment.
Should we get rid of the requestparam annotation as in the interface now?
- Remove swagger related annotations from the controllers (as the documentation layer is moved to the interface level)
- Wind down scope for loggers
- Create external sub package for ArchiverClient
|
simon-ess
left a comment
simon-ess
left a comment
There was a problem hiding this comment.
I might suggest squashing a few of the commits (e.g. when you rename the interfaces separately from creating them), but other than that LGTM.
anderslindho
left a comment
anderslindho
left a comment
There was a problem hiding this comment.
Just noticed that you have a typo in a directory name, @imretoth-ess : respository -> repository
Oh, good catch! I will fix that! |
- Fix type in 'repository' package name
|
|
|
Is there an issue ticket or something with an overview of the changes and the need for it/problem being resolved? |
|
@shroffk the "epic" we're working towards here is enabling the v2 api |
|
Ok, test was just flaky. |
|
can you link the "epic" or something that describes the issues/tasks/motivations? |
|
@shroffk our ticketing system is not accessible from outside of ESS @imretoth-ess can you please update your PR description/top level comment with this content (feel free to adjust as you see fit): This is a preparatory step in order to convert the HTTP API into a RESTful (v2) API. Early outlines for this work can be seen in e.g. #106 (comment). @shroffk is the above clarification sufficient? |
This PR adds repository, API, and service packages, and it splits up the controller code into controller and services. The PR will later be followed by one or more PRs to move business logic from the controller layer to the service layer.
This is a preparatory step in order to convert the HTTP API into a RESTful (v2) API. Early outlines for this work can be seen in e.g. #106 (comment).