-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adds swagger and introduces repository-layer #91
base: development
Are you sure you want to change the base?
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.
67 comments for 134 files :(
Lost concentration multiple times. Not reviewing this again.
@@ -0,0 +1,4 @@ | |||
export default class BuildBuildingRequest { |
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.
Simplify/Remove *Request interfaces?
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.
Why?
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.
Because they are classes without methods - would be an actual good use for a typescript interface type. Also if these stay this short it might be ok to put them into one file together.
Don't remember what else I ment with "remove".
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.
Using interfaces with no defines methods and only properties is somewhat against the definition of an interface, isn't it? Would feel wrong to use interfaces instead of classes here.
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.
In the sense you might know interfaces from other languages, maybe.
But javascript is able to create data objects from pure json like it is the case here. Typescript can use interfaces to type those data objects without the need to instantiate them like you would do with class based objects. IMHO this is actually what typescript interfaces are mostly ment for and obviously this is quite different to other languages like Java.
@@ -0,0 +1,6 @@ | |||
import BuildOrderItem from "../common/BuildOrderItem"; | |||
|
|||
export default class BuildDefenseRequest { |
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.
Simplify/Remove *Request interfaces?
@@ -0,0 +1,6 @@ | |||
import BuildOrderItem from "../common/BuildOrderItem"; | |||
|
|||
export default class BuildShipsRequest { |
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.
Simplify/Remove *Request interfaces?
} | ||
|
||
if (error instanceof DuplicateRecordException || error.message.includes("Duplicate entry")) { | ||
throw new Error(`There was an error while handling the request: ${error.message}`); |
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.
Please explain this error message construction
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
import { Controller, ValidationService, FieldErrors, ValidateError, TsoaRoute, HttpStatusCodeLiteral, TsoaResponse } from 'tsoa'; | ||
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa | ||
import { AuthRouter } from './../routes/AuthRouter'; |
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.
Might be able to put these into routes/index.ts ?
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.
Unfortunately not. This file is generated, you would need to copy/paste the content everytime your api changes
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.
Why wouldn't it be possible to generate imports from routes/index.ts?
src/routes/EventRouter.ts
Outdated
return missionTypeID; | ||
} | ||
} | ||
// import { Response, Router } from "express"; |
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.
Remove file - no outcommented code!
|
||
// TODO: check, if event really is inQueue and/or returning | ||
}); | ||
// import * as chai from "chai"; |
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.
Remove outcommented code
- stage: "Audit" | ||
if: type = pull_request | ||
script: audit-ci --low --report-type full | ||
#- stage: "Audit" |
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.
TODO readd?
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 am not sure, github does auditions out-of-the-box so we could speed up our CI builds by removing the auditing
Kudos, SonarCloud Quality Gate passed!
|
This PR adds a swagger-file and provides a swagger-UI using tsoa (github repo). Moreover, a repository-layer is introduced.
This closes #27 and provides the tools to implement #53 and #54.
TODO for this PR: