refactor(FIR-33559): Remove obsolete account v1 implementation #103
Conversation
ptiurin
changed the title
ptiurin
changed the title
test/unit/v2/database.test.ts
Outdated
| `https://some_system_engine.com/${QUERY_URL}`, | ||
| (req, res, ctx) => { | ||
| const body = (String(req.body) ?? "").toLowerCase(); | ||
| if ( |
There was a problem hiding this comment.
I'd re-write this snippet as following:
if (body.includes("information_schema.engines")) {
if (body.includes("some_engine")) {
return res(ctx.json(selectEngineResponse));
} else if (body.includes("some_other_engine")) {
return res(ctx.json(selectOtherEngineResponse));
} else {
return res(ctx.json(selectEnginesResponse));
}
} else {
return res(ctx.json(selectDbResponse));
}
IMHO it is much more readable. Also then I'd extract the logic to separate function that returns response according to the body and then use it as following:
return res(ctx.json(getResponse(body)))';
There was a problem hiding this comment.
I don't think nested ifs are much more readable either. However, this test is much simpler - the setup in question was copied over from the previous test. We only need one case in this test so no nesting is necessary. I've removed redundant lines.
| accountVersion >= 2 | ||
| ? this.CREATE_PARAMETER_NAMES_V2 | ||
| : this.CREATE_PARAMETER_NAMES; | ||
| const queryParameters: (string | number | boolean)[] = []; |
There was a problem hiding this comment.
just not sure why boolean has been added here. I believe that is correct, but it is not clear why it was not in previous implementation and exists here.
There was a problem hiding this comment.
This is probably a mistake. I think that boolean is first used by AUTO_START option, which was added recently. Yet, it's good we updated this annotation
There was a problem hiding this comment.
Yeah, this was confusing me as well. I think the type checker was only checking one condition here? In any case, this is corrected now.
| accountVersion >= 2 | ||
| ? this.CREATE_PARAMETER_NAMES_V2 | ||
| : this.CREATE_PARAMETER_NAMES; | ||
| const queryParameters: (string | number | boolean)[] = []; |
There was a problem hiding this comment.
This is probably a mistake. I think that boolean is first used by AUTO_START option, which was added recently. Yet, it's good we updated this annotation
|
tests: https://github.com/firebolt-db/firebolt-node-sdk/actions/runs/9495933440