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

discussion: operation construction code cleanup trade-offs #764

Closed
dtkav opened this issue Nov 9, 2018 · 3 comments
Closed

discussion: operation construction code cleanup trade-offs #764

dtkav opened this issue Nov 9, 2018 · 3 comments

Comments

@dtkav
Copy link
Collaborator

dtkav commented Nov 9, 2018

Description

This is a ticket to compare a couple refactor PRs.
Right now there is iffing and elsing on the spec version within apis/abstract.py.
Ideally this coupling should not exist, and should be contained within the Specification class / interface.
@cziebuhr and I have been working through some ways to refactor a bit.

Comparison

@cziebuhr and I worked through a few ideas for doing code cleanup.
One approach was #726 which doesn't change the operation interface, and instead adds some convenience constructors to reduce coupling.
This approach goes to great lengths to avoid passing the entire spec to the operation, so it has a wider, but flatter interface. Any parts of the spec that aren't within the "operation" scope are passed in as separate arguments (e.g. app_security). This is how the Operation class was before I split it, so I just continued with the wide interface.

The other approach is #730 which passes the entire spec to the operation. This means that the operation class is more coupled to the spec (relies on parts outside of the operation scope). This is a skinnier, but deeper interface.
The implications are that it might be harder to use/test the operations classes on their own without having a full spec. It's also fewer lines of code, and more readable. Finally, it changes the operation interface, so is backwards incompatible (It's hard to know how 'public' this interface is, and what the impact of breaking compatibility would be).

What's next?

It would be nice to land some of this cleanup. I don't feel particularly attached to either one.
I think my preference would be to land #726 since it's ready and then continue to discuss/refine #730 since it can build on #726 (It's a bigger refactor with higher risk and reward).

Let me know what you all think.

@dtkav
Copy link
Collaborator Author

dtkav commented Nov 15, 2018

FYI I think #730 was closed automatically because the dev-2.0 branch was deleted.
Either way - @jmcs can you review #726 ? I think it's worth getting at least that bit of cleanup in.

@dtkav
Copy link
Collaborator Author

dtkav commented Dec 17, 2018

Hey @cziebuhr - are you still interested in rebasing #730 ?
Let me know - otherwise I'll close out this discussion ticket.

@cziebuhr
Copy link
Contributor

Interested yes, but I currently don't find time to do it.

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

3 participants