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

Remove register_setup_step macro #486

Conversation

matthewmcgarvey
Copy link
Member

Connected to #453

In order to support views well, we need to not generate the operation code for view classes. The way that the code was setup, there was no easy way to avoid setting it up. Since this was not a documented API, I believe it is safe to remove.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

I'm not sure what the original intention of this was. Maybe just a way to ensure that everything runs? I think it's fine to remove too since this isn't something that needs to be user accessible. It's more like an internal setup deal.

@matthewmcgarvey matthewmcgarvey merged commit 37e6475 into luckyframework:master Oct 18, 2020
@matthewmcgarvey matthewmcgarvey deleted the remove-register-setup-step branch October 18, 2020 02:09
Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Sorry for looking at this so late. The point of register_setup_step was that third party libraries could register new setup steps. Maybe setting up new methods, or generating classes like the Acram:SaveOperationTemplate.setup.

I just never got around to documenting it, but it would be pretty powerful.

My thought would be that we add register_setup_step again, but only register the steps where needed. That may mean we need to rework register_setup_step to work on aper-model basis though.

So summary is this is fine as-is since I don't think anyone was using it, but ideally we'd add some version of it that works per-model and document it so it can be used by third party libs. For example you might include Shrine::Model and it'll add some code to your operation automatically, or add some methods that are for handling uploads.

This is not a high priority but just wanted to share the background and a possible solution

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

Successfully merging this pull request may close these issues.

3 participants