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.

Sign up for GitHub

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

Jump to bottom

simplify Operation class construction #726

Merged
merged 3 commits into from
Nov 15, 2018
Merged

simplify Operation class construction #726

merged 3 commits into from
Nov 15, 2018

Conversation

dtkav
Copy link
Collaborator

@dtkav dtkav commented Oct 18, 2018 •
edited
Loading

Follow up to #713

Changes proposed in this pull request:

  • Introduce make_operation that creates the correct Operation class from a Specification
  • Move Specification classes to separate file (spec.py)

@dtkav dtkav changed the title simplify *Operation creation simplify Operation class construction Oct 19, 2018
@dtkav dtkav mentioned this pull request Oct 19, 2018
Closed
@dtkav dtkav force-pushed the make_operation branch 2 times, most recently from 6f71d70 to 1d6f7a0 Compare October 19, 2018 06:43
@hjacobs hjacobs added this to the 2.0 milestone Oct 19, 2018
@dtkav
Copy link
Collaborator Author

dtkav commented Oct 19, 2018

Hey @cziebuhr , some more refactor - let me know what you think!

@cziebuhr
Copy link
Contributor

I would have gone even further, to pass and save the spec object in the operation instead of single parameters in the operation constructor.

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 19, 2018

I think the current PR is as far as I can go without breaking interfaces or changing tons of tests.

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 19, 2018

do you want to put up a PR of what you're thinking?

@cziebuhr
Copy link
Contributor

do you want to put up a PR of what you're thinking?

I can do that

2 similar comments
@cziebuhr
Copy link
Contributor

do you want to put up a PR of what you're thinking?

I can do that

@cziebuhr
Copy link
Contributor

do you want to put up a PR of what you're thinking?

I can do that

@cziebuhr cziebuhr mentioned this pull request Oct 23, 2018
Closed
@dtkav dtkav changed the base branch from dev-2.0 to master November 9, 2018 02:48
@dtkav dtkav mentioned this pull request Nov 9, 2018
Closed
@spec-first spec-first deleted a comment Nov 9, 2018
@spec-first spec-first deleted a comment Nov 9, 2018
@spec-first spec-first deleted a comment Nov 9, 2018
@spec-first spec-first deleted a comment Nov 13, 2018
@dtkav dtkav requested a review from jmcs November 15, 2018 04:39
@dtkav dtkav added the ready label Nov 15, 2018
@dtkav dtkav removed this from the 2.0 milestone Nov 15, 2018
@spec-first spec-first deleted a comment Nov 15, 2018
@jmcs
Copy link
Contributor

jmcs commented Nov 15, 2018

👍

@jmcs jmcs merged commit 72b1667 into spec-first:master Nov 15, 2018
@RobbeSneyders RobbeSneyders mentioned this pull request Jul 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmcs jmcs jmcs approved these changes

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtkav @cziebuhr @jmcs @hjacobs