ClickHouse client & logger abstraction#4
Conversation
|
hi @rykdesjardins , thank you for PR, I will review it in a bit! |
bologer
left a comment
bologer
left a comment
There was a problem hiding this comment.
Sorry for delay in review, was quit buys these days.
Here is another small comment:
This is just matter of taste, but I would appreciate if you would remove Like from all type names and keep without it. For example, LoggerLIke would be Logger, same for ClickHouseLike and QueryCursorLike.
In addition, can you add new version (v2.0.0) to README.md and describe how to migration steps from old way to new way. This will definitely help people who plan to migrate. Also, near the release version, just write placeholder date, e.g. XXXX-XX-XX, I will updated it myself when would be preparing for release.
Once you will adjust code, I will prepare the release and would be happy to roll it out.
| "toc": "./node_modules/.bin/markdown-toc README.md -i " | ||
| }, | ||
| "devDependencies": { | ||
| "@clickhouse/client": "^0.2.9", |
There was a problem hiding this comment.
Are you sure you mean to put @clickhouse/client into dev dependencies? Isn't it supposed to be in dependencies?
| return { | ||
| async toPromise() { | ||
| const cursor = await client.query({ | ||
| query, | ||
| query_params: reqParams | ||
| }); | ||
|
|
||
| return cursor.json() as Promise<Object[]>; | ||
| } | ||
| }; |
There was a problem hiding this comment.
What do you think if we remove toPromise method and return Promise immediately? This way we can avoid extra method call and code would look cleaner/shorter. The query (e.g. query<T[]>(...): Promise<T[]> method can have new generic which would represent returned type.
Let me know what you think about it?
| private orderByPart: Array<string> = []; | ||
|
|
||
| constructor(ch: ClickHouse, logger: Logger | null) { | ||
| constructor(ch: ClickHouseLike | null, logger: LoggerLike | null) { |
There was a problem hiding this comment.
Is there a specific reason you've made ch argument optional? It must be required, otherwise it's class itself is pointless.
2 participants
Pretty solid query builder. Thanks for your open-source contribution.
I noticed the client and logger expected by the QueryBuilder class only accepted the
ClickHouseclient from theclickhouselibrary, and the logger had to bewinston.This PR allows passing any ClickHouse client, as long as it respects the
ClientHouseLikeinterface. I made sure that both the ClickHouse client abstraction and logger abstraction respect the previous concretions so that it's fully backwards compatible.Was planning on using your library, but I'm using the official NodeJS Client. This PR will allow anyone to use your library, no matter what client and logger they're using. Also comes with a driver for the official client. Didn't make one for the
ClickHousefromclickhousebecause it already respects the requested interface.Also removed unused variables from
src/InsertQuery.ts, otherwise I was getting warnings.