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

preEmit and shouldEmit in actions #64

Closed
krawaller opened this issue Sep 10, 2014 · 2 comments
Closed

preEmit and shouldEmit in actions #64

krawaller opened this issue Sep 10, 2014 · 2 comments

Comments

@krawaller
Copy link
Contributor

Do we really need both preEmit and shouldEmit? Code from preEmit could just as easily be put inside shouldEmit instead.

I see the values of the explanatory names and not having to actively return true if you just want to run code pre-emission, but is that worth muddying the API?

As for having to return true to proceed, we could switch it so that emission is cancelled if return value is true.

@spoike
Copy link
Member

spoike commented Sep 10, 2014

There is currently a little implementation mistake with preEmit... it really should take a return value and emit that value as argument instead. If undefined it should just pass through the arguments as the action got it.

See #58

@krawaller
Copy link
Contributor Author

Excellent discussion there, didn't think to search first, sorry!

And allowing preEmit to morph the data (clever idea!) of course justifies the existence of shouldEmit.

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

No branches or pull requests

2 participants