Skip to content
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

Open
wants to merge 52 commits into
base: development
Choose a base branch
from

Conversation

mamen
Copy link
Contributor

@mamen mamen commented Aug 11, 2020

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:

  • Fix sonar issues
  • Write tests for repository-layer

@mamen mamen added feature refactoring Code needs refactoring architecture labels Aug 11, 2020
Copy link
Contributor

@Phoscur Phoscur left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify/Remove *Request interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

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".

Copy link
Contributor Author

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.

Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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}`);
Copy link
Contributor

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';
Copy link
Contributor

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 ?

Copy link
Contributor Author

@mamen mamen Aug 18, 2020

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

Copy link
Contributor

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?

return missionTypeID;
}
}
// import { Response, Router } from "express";
Copy link
Contributor

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";
Copy link
Contributor

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO readd?

Copy link
Contributor Author

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

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants