Skip to content
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

use serialize method in transform command #64

Merged
merged 5 commits into from Jun 29, 2021

Conversation

danjuv
Copy link
Contributor

@danjuv danjuv commented Jun 16, 2021

This is small change, but the intention is to add a serialize member function that is called when transforming commands into their serialized counterparts.

The intention is when we add a meta-command we can add it as a subclass of Command and override serialize to add in the path to the wrapper script.

@danjuv danjuv marked this pull request as draft June 16, 2021 03:51
@danjuv danjuv marked this pull request as ready for review June 16, 2021 03:51
Copy link
Owner

@joscha joscha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks for the PR - how is the proposed serialize function (currently public) different from just overriding toString in a subclass?

The only thing I can think of is that potentially the implementations of toString and serialize would be different. Taking your example in #65 e.g. in structural output we could simplify RetryCommand('yarn', 2) to yarn [retry 2] whereas in serialize it could return yarn || yarn.

So in that sense the addition of serialize LGTM, however can we make sure that the visibility serialize is class-protected? You will most likely need to move transformCommand into the Command class as a static and maybe access it via a Symbol() in order to only make it accessible from within buldkite-graph

@joscha joscha requested a review from uri-canva June 21, 2021 08:01
src/steps/command.ts Outdated Show resolved Hide resolved
src/steps/command.ts Outdated Show resolved Hide resolved
src/steps/command.ts Outdated Show resolved Hide resolved
src/steps/command.ts Outdated Show resolved Hide resolved
src/steps/command.ts Outdated Show resolved Hide resolved
Copy link
Owner

@joscha joscha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, let's try and use this in #65 - once we have a working sample we can merge both together and release a new version

@danjuv danjuv mentioned this pull request Jun 28, 2021
@joscha joscha merged commit 83c9c52 into joscha:master Jun 29, 2021
@joscha
Copy link
Owner

joscha commented Jun 29, 2021

🎉 This PR is included in version 5.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants