Update to Grain.stack_grains per aimsgb deprecation warning #67
Conversation
Along with some signature and docstring updates
|
I can confirm that the warning message disappeared. So the changes fixed the issue. Still I am wondering how we want to handle these kind of user facing API changes in the future. Should we release a new major version |
| vacuum=0.0, | ||
| gap=0.0, | ||
| delete_layer="0b0t0b0t", | ||
| tol=0.25, | ||
| to_primitive=False, | ||
| add_if_dist=None, |
There was a problem hiding this comment.
Is it necessary to change the order of the arguments?
There was a problem hiding this comment.
Not necessary, but beneficial.
Previously the arguments were GrainBoundary.__init__ args, then an arbitrary mixture of GrainBoudary.__init__ and GrainBoundary.build_gb kwargs. This rationalizes things so that it's GrainBoundary.__init__ args, GrainBoundary.__init__ kwargs, Grain.stack_grains kwargs, then deprecated kwargs, with user-facing kwargs appearing in the same order as they do in the underlying library.
Since we need to change the signature anyways to handle the new kwarg and the renamed kwarg, I thought we might as well tighten the API up at the same time.
Co-authored-by: Jan Janssen <[email protected]>
Yeah, this is a good thought. On google I found the recommendation:
In this case we only re-order existing keyword arguments and introduce a new one and a newly-named one (the deprecated keyword still works), so I would claim this falls under "without breaking the existing API" such that minor is sufficient. If you instead think that the re-ordering is sufficient to warrant a major bump I would be content with that too. |
Along with some signature and docstring updates.
Closes #65