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

Move cmd and with_job to FlowProject.operation #679

Merged
merged 12 commits into from
Oct 6, 2022

Conversation

b-butler
Copy link
Member

@b-butler b-butler commented Oct 5, 2022

Description

Does not break current behavior.

  • refactor: Deprecate cmd and with_job decorators.
  • feat: Add cmd and with_job keyword only arguments for @operation
  • test: Add tests for new keyword only arguments of FlowProject.operation

Motivation and Context

Closes #669
and replaces #671.

I had large suggestions to the way the code should be changed. I think this is much simpler to get the desired behavior.

Checklist:

@b-butler b-butler requested review from a team as code owners October 5, 2022 16:26
@b-butler b-butler requested review from cbkerr and pepak13 and removed request for a team October 5, 2022 16:26
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In the future, it can be helpful to do something like this instead:

def _func(...):
    # do stuff

def func(...):
    warnings.warn(FutureWarning, "func is deprecated")
    _func(...)

and then call _func internally, instead of using a warnings.catch_warnings + warnings.simplefilter.

Also, check it with_job is used with a non-default aggregation rather
than if an aggregator is specified as this is the best way to avoid
errors.
@b-butler
Copy link
Member Author

b-butler commented Oct 6, 2022

@bdice I agree and it is generally what I do. Here, I think the solution is different in that I think we should remove the implementations in flow/operations.py and put them in operation register.

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #679 (30405b7) into master (7a33919) will increase coverage by 0.08%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   80.15%   80.24%   +0.08%     
==========================================
  Files          32       32              
  Lines        3165     3189      +24     
  Branches      674      682       +8     
==========================================
+ Hits         2537     2559      +22     
- Misses        497      498       +1     
- Partials      131      132       +1     
Impacted Files Coverage Δ
flow/aggregates.py 87.55% <ø> (ø)
flow/operations.py 80.70% <90.00%> (-0.55%) ⬇️
flow/project.py 82.69% <100.00%> (+0.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@b-butler b-butler merged commit 7c11e6e into master Oct 6, 2022
@b-butler b-butler deleted the feat/add-args-to-operation branch October 6, 2022 18:44
@b-butler b-butler added this to the v0.22.0 milestone Oct 13, 2022
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.

Allow users to pass with_job and cmd through operation decorator
3 participants