-
Notifications
You must be signed in to change notification settings - Fork 0
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
take timedelta kwargs in after_deploy #61
Comments
If the goal is to avoid imports, what about a separate method? If that doesn't strike your fancy, then I can on board with specific kwargs or the variable kwargs being for timedelta. |
If we were to write a migration generator, which version would be the canonical representation? |
Both, no? |
I think my question here wasn't clear. If we had both Also, #68 is relevant to this, because the most likely implementation I see for that is to add a keyword argument to the classmethod. I think we are unlikely to want a keyword argument that would conflict with If we do add this, I think I'd prefer specific or variable kwargs over the additional method. In no small part because I think anything we would want to add to this API would likely be in both methods, so it's two methods worth of signatures we'd have to keep in sync. |
Additional imports in migrations are cumbersome, but not necessarily unreasonable. The way that I can see to avoid them is to take the same keyword arguments in the
after_deploy
classmethod, and automatically create the timedelta if they are given. Depending how much we like it, we could even drop thedelay=
kwarg in favor of those keyword arguments, since we haven't released the new API yet.The
delay
argument feels like the correct data type. I generally try to avoid passing along kwargs like that, to avoid challenges where the keys might overlap. But so far I'm not imagining any use for additional keyword argument in any of theSafe
classmethods.So I'm a little ambivalent on it, but I feel like it might be appropriate to take a little bit of risk in order to simplify the DX by just supporting the keyword arguments directly. What do you think?
The text was updated successfully, but these errors were encountered: