-
-
Notifications
You must be signed in to change notification settings - Fork 620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add makeUsingStub
to BaseCommand
#4147
Conversation
class StubGenerator { | ||
#command: BaseCommand | ListCommand | ||
#flags: Record<string, any> | ||
|
||
constructor(command: BaseCommand, flags: Record<string, any>) { | ||
this.#command = command | ||
this.#flags = flags | ||
} | ||
|
||
async generate(stubsRoot: string, stubPath: string, stubState: Record<string, any>) { | ||
const stub = await this.#command.app.stubs.build(stubPath, { source: stubsRoot }) | ||
const output = await stub.generate(Object.assign({ flags: this.#flags }, stubState)) | ||
|
||
const entityFileName = slash(this.#command.app.relativePath(output.destination)) | ||
const result = { ...output, relativeFileName: entityFileName } | ||
|
||
if (output.status === 'skipped') { | ||
this.#command.logger.action(`create ${entityFileName}`).skipped(output.skipReason) | ||
return result | ||
} | ||
|
||
this.#command.logger.action(`create ${entityFileName}`).succeeded() | ||
return result | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this class? Can't we just inline all the generate
method code inside the makeUsingStub
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to avoid duplicating the code in the ListCommand class that implements BaseCommand as I explain in my comment above
As it is the case for the terminate
function for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, I see. I think, we will run into this issue more often as we try to expand the capabilities of the base command.
Should we fix it right now maybe using mixins?
Proposed changes
Add a method
makeUsingStub
to Ace BaseCommand.Further comments
Not sure about this PR. It doesn't seem very clean to me because we have a particular OOP architecture here and so I was forced to duplicate the code in
ListCommand
, and also to leave thestubsGenerator
property in public. (that's also why I preferred to using composition here by extracting the StubGenerator class)I almost went further and refactored it maybe with Mixins (to remove the fact that ListCommand implements BaseCommand) because it seems to be problematic
But I prefered to keep it simple and just duplicate for the moment because it's a very small change
Let me know what you think!