-
Notifications
You must be signed in to change notification settings - Fork 43
refactor: refactor envbuilder to use coder/serpent as CLI engine #140
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
Conversation
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.
I think you can review clidocgen in coder/coder and try to implement something similar or extract the clidocgen to a common /library repo?
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.
It probably makes sense to use coder/serpent
here as what I'm seeing is essentially a reimplementation of a subset of its existing functionality.
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.
I believe you are heading in the right direction. Most of my comments pertain to adhering to the Go convention, but the overall concept is correct.
I suggest narrowing the scope of this PR to solely adopting coder/serpent
(what you have now) and then preparing a follow-up to implement clidocgen
. What do you think?
Marcin is going to be on PTO next week
@mafredri @johnstcn @dannykopping I think this PR is in a good state to get merged. Here are some followups from the reviews:
I'm planning to create an issue + PR for each one of them after merging #147. |
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.
LGTM
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.
Related to #130