refactor: commands into command classes #424
Conversation
erunion
requested review from
kanadgupta,
a team,
Dashron and
julshotal
and removed request for
a team
| is_hidden: promptResponse.is_stable ? false : !(isPublic === 'true' || promptResponse.is_hidden), | ||
| }), | ||
| }) | ||
| .then(handleRes) |
There was a problem hiding this comment.
Moved this over to use handleRes because we can here.
|
|
||
| exports.run = () => { | ||
| if (!configStore.has('email') || !configStore.has('project')) { | ||
| return Promise.reject(new Error(`Please login using \`${config.get('cli')} ${loginCmd.command}\`.`)); |
There was a problem hiding this comment.
I stopped directly calling the login command for its name here because we know it. It's login.
| exports.run = async function (opts) { | ||
| let { email, password, project, token } = opts; | ||
| /* istanbul ignore next */ | ||
| async function getCredentials() { |
There was a problem hiding this comment.
Since I've moved this inside of run we don't need to pass it opts anymore.
| } | ||
|
|
||
| if (version === undefined) { | ||
| return Promise.resolve(VersionsCommand.getVersionsAsTable(versions)); |
There was a problem hiding this comment.
Since getVersionsAsTable is a static method we need to call it off the main class, this.getVersionsAsTable won't work. The method doesn't really need to be static, but since it doesn't use any internal command data it probably makes sense for it to be static?
If not, we'll need to alias this to self at the top of run to get around scope issues within the fetch thenable that this line is at.
| exports.position = openapi.position + 1; | ||
| module.exports = class SwaggerCommand extends OpenAPICommand { | ||
| constructor() { | ||
| super(); |
| is_hidden: promptResponse.is_stable ? false : !(isPublic === 'true' || promptResponse.is_hidden), | ||
| }), | ||
| }) | ||
| .then(handleRes) |
| @@ -2,6 +2,8 @@ | |||
| "extends": ["@readme/eslint-config"], | |||
| "root": true, | |||
| "rules": { | |||
| "class-methods-use-this": ["error", { "exceptMethods": ["run"] }], | |||
There was a problem hiding this comment.
Wanna add a small comment explaining why we need this?
|
|
||
| commands.push({ | ||
| file, | ||
| command, | ||
| command: new Command(), |
There was a problem hiding this comment.
Thoughts on new here and load vs. module.exports = new (commandClass)(); in each file?
I've used both techniques in the past. new here and load is nice to ensure that we don't have side effects leaking between calls, and might be what I prefer, but I figured I'd bring it up if you thought it would be useful.
There was a problem hiding this comment.
I think I prefer to do the instantiation on load instead of in the export.
|
I def like the classes, clean and allows for interesting, clean improvements like inheritance. |
| @@ -52,7 +54,9 @@ describe('rdme versions*', () => { | |||
| .basicAuth({ user: key }) | |||
| .reply(200, [versionPayload, version2Payload]); | |||
|
|
|||
| await expect(versions.run({ key })).resolves.toMatchSnapshot(); | |||
| const table = await versions.run({ key }); | |||
There was a problem hiding this comment.
Reverted this back to using these two assertions because the FORCE_COLOR env variable doesn't seem to always get used within the colors dependency of cli-table.
| coverage | ||
| node_modules/ | ||
| coverage/ | ||
| swagger.json |
There was a problem hiding this comment.
We technically shouldn’t need this since a successful test run will result in this file being deleted properly… but I suppose doesn’t hurt!
|
Also wait we probably don't need both of these right 👀 Lines 25 to 28 in 2bd4199 rdme/__tests__/set-node-env.js Lines 1 to 3 in 2bd4199 |
|
Ah good eye I'll remove the one in the workflow config. |
🧰 Changes
While thinking around with Typescript here this evening, in order for TS to understand each command I began migrating every command into a formal class that implements a core
Commandinterface.Because I quite liked how that work felt, having command code nestled inside of a more formal class environment, I've reworked that work here so that every command has a constructor where it sets up its arguments and
--helpscreen docs and an asyncrunmethod to run the command.If I manage to get the Typescript work reviewable it'll make that PR a lot easier to read with this refactoring out of the way.
🧬 QA & Testing