Feature/random data provider #42
Conversation
… dirty, needs some more work.
…SO's) and made Dockerfile install SGX stuff in the container.
…andom-data-provider
…om-data-provider # Conflicts: # src/domain/common/usecase/FetchDataUseCase.js # src/domain/request/usecase/ExecuteReadyRequestsUseCase.js # src/index.js ...and introduced DataFetcherFactory along with a merge commit, like a complete idiot
…actor/abstractonise-data-fetchers
…+ other CR changes
…om-data-provider # Conflicts: # src/application/dataFetcher/AxiosUrlDataFetcherAdapter.ts # src/domain/common/usecase/FetchDataUseCase.spec.ts # src/domain/common/usecase/FetchDataUseCase.ts # src/domain/request/usecase/ExecuteReadyRequestsUseCase.spec.ts # src/domain/request/usecase/ExecuteReadyRequestsUseCase.ts # src/index.ts
…om-data-provider # Conflicts: # src/domain/request/requestExecutor/RequestExecutorFactory.ts
…om-data-provider # Conflicts: # src/domain/request/requestExecutor/RequestExecutorStrategy.spec.ts # src/domain/request/usecase/ExecuteReadyRequestsUseCase.ts # src/index.ts
kss-espeo
requested review from
kwedrowicz,
przemyslawjanpietrzak and
tcichowicz
# Conflicts: # src/application/dataFetcher/AxiosUrlDataFetcherAdapter.ts # src/domain/request/requestExecutor/RequestExecutorStrategy.spec.ts # src/domain/request/requestExecutor/RequestExecutorStrategy.ts # src/domain/request/requestExecutor/UrlRequestExecutor.ts # src/index.ts
Dockerfile
Outdated
| apt-get install curl libssl1.0-dev --yes | ||
|
|
||
| # Install nodejs v11 | ||
| RUN curl -sL https://deb.nodesource.com/setup_11.x | bash - |
There was a problem hiding this comment.
Can we use NodeJS version 10.16? It's the latest LTS.
package.json
Outdated
| @@ -59,8 +61,12 @@ | |||
| "devDependencies": { | |||
| "@types/dotenv": "6.1.1", | |||
| "@types/express": "4.17.0", | |||
| "@types/ffi": "^0.2.2", | |||
There was a problem hiding this comment.
Target dependencies strictly.
| import RequestUrlParser from '@core/domain/request/RequestUrlParser'; | ||
| import DataFetcher from '../../domain/common/port/DataFetcherPort'; |
There was a problem hiding this comment.
Why you added ../../ instead of @core?
| import RandomDotOrgDataFetcherAdapter from './RandomDotOrgDataFetcherAdapter'; | ||
| import {response} from './RandomDotOrgResponse'; | ||
|
|
||
| describe('Random.org Integration Test', () => { |
There was a problem hiding this comment.
Why this test suite is an integration one? I see that you mock here random api.
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| data: `{"jsonrpc":"2.0","method":"generateIntegers","params":{ |
There was a problem hiding this comment.
Please format this property properly as an object and then convert to string if needed.
| id: 21686, | ||
| }; | ||
|
|
||
| export {response}; |
There was a problem hiding this comment.
Did we talk that we want to move all test fixtures to separate directory?
| @@ -16,6 +16,14 @@ class Request { | |||
| getSelectionPath() { | |||
| return RequestUrlParser.resolveSelectionPath(this.url); | |||
| } | |||
|
|
|||
| getLeftSideBound() { | |||
There was a problem hiding this comment.
I'm not sure if it's the right place for this. Not every request would have left and right side bounds (but from the code it would look like this).
There was a problem hiding this comment.
It is not. I will move RequestUrlParser logic to RequestStrategy, along with this as well. I intend to run an enhancement task that will address couple of things like this plus add more tests around random provider feature. However I decided to postpone it because we have a feature to deliver.
| @@ -28,6 +28,26 @@ class RequestUrlParser { | |||
|
|
|||
| return wrappedUrl.substr(pathStartIndex); | |||
| } | |||
|
|
|||
| static resolveLeftSideBound(wrappedUrl: string): string { | |||
| } | ||
|
|
||
| canHandle(contentType: string): boolean { | ||
| return 'random' === contentType; |
There was a problem hiding this comment.
This string should be extracted to some enum.
There was a problem hiding this comment.
Agreed. This requires changes in many places (biggest offender is RequestUrlParser) . I plan to do that as a part of enhancement task.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
No description provided.