feat: move on 'v3' format #58
Conversation
alex-gilin
force-pushed
the
v3
branch
2 times, most recently
from
4746782
to
e6d8853
Compare
| await cfGetServiceInstances({ 'filters': [{ key: eFilters.service_plan_guid, value: _.join(await getServicePlansGuidList(serviceTypes)), op: eOperation.IN }] }), | ||
| (service => { return _.includes(serviceTypes, service.serviceName); }) | ||
| ); | ||
| const services = await cfGetServices(await padQuerySpace({ 'filters': [{ key: eFilters.names, value: _.join(_.map(serviceTypes, encodeURIComponent)) }] })); |
There was a problem hiding this comment.
This is not clear when all in one line. I would divide it and maybe add comments.
There was a problem hiding this comment.
Disagrees with you - declarative style more clear and compact -> personal preference.
src/messages.ts
Outdated
| export const messages = { | ||
| space_not_set: "Space not set", | ||
| service_creation_started: "", | ||
| service_creation_started: "Service creation started, waiting for 'Ready' state.", |
There was a problem hiding this comment.
Is it visible for a user? Is reviewed by Paola?
src/utils.ts
Outdated
| import * as _ from "lodash"; | ||
| import * as os from "os"; | ||
| import * as fsextra from "fs-extra"; | ||
| import { IServiceQuery, CF_PAGE_SIZE } from "./types"; | ||
| import path = require("path"); |
There was a problem hiding this comment.
Will "import * as path from 'path';" more consistent here?
| const configJson = parse(await fsextra.readFile(cfGetConfigFilePath(), "utf8")); | ||
| return _.get(configJson, `${field}`); | ||
| } catch (error) { | ||
| // empty or non existing file |
There was a problem hiding this comment.
No action on error? What will be returned? Does the calling function expect it?
There was a problem hiding this comment.
sure. it is by design.
src/cli.ts
Outdated
| @@ -55,6 +49,7 @@ export class Cli { | |||
| private static readonly CF_CMD = "cf"; | |||
|
|
|||
| private static cliResultOnExit(stdout: string, resolve: (value?: CliResult | PromiseLike<CliResult>) => void, stderr: string, code: number) { | |||
| const getValueExceptEol = (value: string) => value === '\n' ? '' : value; | |||
There was a problem hiding this comment.
remove this line (see comment on line 79)
| @@ -50,29 +45,29 @@ describe("cf-local-a unit tests", () => { | |||
| exitCode: 1 | |||
| }; | |||
|
|
|||
| it("stdout is not empty, authentication is OK", async () => { | |||
| it("success:: stdout is not empty, authentication is OK", async () => { | |||
There was a problem hiding this comment.
why not ?see below.
| cliResult.stdout = "test modified (current test"; | ||
| cliResult.error = ""; | ||
| cliMock.expects("execute").withExactArgs(["targets"], undefined, undefined).resolves(cliResult); | ||
| const result = await cfLocal.cfGetTargets(); | ||
| expect(result).to.be.deep.equal([{ label: "test modified", isCurrent: true, isDirty: true }]); | ||
| }); | ||
|
|
||
| it("no '(current' in parentthesisPos", async () => { | ||
| it("ok:: no '(current' in parentthesisPos", async () => { |
There was a problem hiding this comment.
what is this prefix "ok::" used for ?
There was a problem hiding this comment.
test description combined according to the next pattern : 'result':: 'description'-'optional params', where
'result' - 'ok' | 'fail' | 'exception'
'description' - free text explains test purpose/case
'optional params' - optional to different test cases
In general, I intend to use this pattern in all unit test for giving more flexible options to filter/search different test units.
No description provided.