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

allow editing op name and args after init #245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pwyllcrusader
Copy link

@pwyllcrusader pwyllcrusader commented Sep 2, 2024

Description

Related Issues

#244

Progress

Pull request checklist

  • Tests: This PR includes tests for covering the features or bug fixes (if applicable).
  • Docs: This PR updates/creates the necessary documentation.
  • CI: Make sure your Pull Request passes all CI checks. If not, clarify the motif behind that and the action plan to solve it (may reference a ticket)

How to test it

Visual reference

@pwyllcrusader
Copy link
Author

pwyllcrusader commented Sep 4, 2024

@barbieri sorry, I'm a bit new to such activities. is there anything else I have to do in order to add getter/setters to Operation class?

@barbieri
Copy link
Member

let's see if the tests pass... I'm bit out of time these days, so may take a while for me to come back with anything more concrete about your code.

All in all, as I said in #244 I think it would be better to create a function that takes the parameters and create the Operation object instead of mutating it.

@pwyllcrusader
Copy link
Author

pwyllcrusader commented Sep 10, 2024

let's see if the tests pass... I'm bit out of time these days, so may take a while for me to come back with anything more concrete about your code.

All in all, as I said in #244 I think it would be better to create a function that takes the parameters and create the Operation object instead of mutating it.

That's how I did but this approach adds one (or 2, don't quite remember) more line per test. If there are hundreds of them it's quite significant. Definitely it's not critical but adding setters sounded to me like not big deal as well

@barbieri
Copy link
Member

ok, we'll need to add a test to cover it, as you can see the coverage dropped and we need to keep it 100% for that file.

@barbieri
Copy link
Member

(I rebased your branch using github UI, I had to do it since 3.7 tests were broken, so did move to python >=3.8 and bumped other deps... fetch, rebase and reinstall all your deps with poetry)

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.

2 participants