StarChart-Labs / corona-ide

A exploratory project to build a lighter, simpler Java IDE - or learn trying!
Eclipse Public License 1.0
3 stars 1 forks source link

#71 : Add Project RestServer, refactor project delete #73

Closed nickavv closed 6 years ago

nickavv commented 6 years ago

To make deletion more compatible with a RESTful access mentality I refactored it into one method which takes a request, which contains a boolean for whether or not to remove from disk.

codecov[bot] commented 6 years ago

Codecov Report

Merging #73 into master will decrease coverage by 4.24%. The diff coverage is 53.65%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #73      +/-   ##
===========================================
- Coverage     85.45%   81.2%   -4.25%     
- Complexity      109     113       +4     
===========================================
  Files            23      25       +2     
  Lines           385     415      +30     
  Branches         33      37       +4     
===========================================
+ Hits            329     337       +8     
- Misses           43      63      +20     
- Partials         13      15       +2
Impacted Files Coverage Δ Complexity Δ
...a/com/coronaide/main/server/ProjectRestServer.java 0% <0%> (ø) 0 <0> (?)
.../java/com/coronaide/core/model/ProjectRequest.java 100% <100%> (ø) 6 <1> (ø) :arrow_down:
...com/coronaide/core/model/ProjectDeleteRequest.java 35.29% <35.29%> (ø) 3 <3> (?)
...om/coronaide/core/service/impl/ProjectService.java 90.54% <93.75%> (-5.05%) 13 <4> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 11cccfd...97be7fb. Read the comment docs.

BlackDuckCoPilot commented 6 years ago

Black Duck Security Report

Merging #73 into master will not change security risk.

Added Components

Clean: 3

Removed Components

Clean: 3

Click here to see full report

romeara commented 6 years ago

@nickavv I think for delete, it might make more sense to not take a request - maybe query parameter for the disk state (default false)

nickavv commented 6 years ago

@romeara Typically I'd agree, and I'd make the endpoint be DELETE at /projects/{id}?removeFromDisk=bool

But the unique ID for projects is their workspace path, which is hard to put into a URI (could encode or hash it but it gets messy). So it made more sense to me to send it in the body, and at that point why not send the other parameter of the request in the same body?

romeara commented 6 years ago

@nickavv We may want to give them an actual ID - the path being the identifier worked before, but that was before we ended up using a RESTful-style external interface, so we'll likely need to tweak our strategy to account for the new constraints

nickavv commented 6 years ago

I'm perfectly happy with everything having a nice restful identifier, and I'll file a ticket for the backend for that. The nice thing is since the "api" is exclusively for consumption by the frontend and they will be bundled together we don't have to version the APIs, so we can just change that later.