Add operation.with_directives decorator #502

b-butler · 2021-04-20T20:32:30Z

Description

Adds a new decorator which add the appropriate directives along with registering the function as an operation FlowProject.operation.with_directives.

To accomplish this, we create a new class OperationRegister, that is specific to a FlowProject class like the preconditions and postconditions classes. We use the __call__ magic method for the basic decorator.

Motivation and Context

Addresses: #309

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog.

The new decorator adds the appropriate directives along with registering the function as an operation. To accomplish this, we create a new class operation, that is specific to a ``FlowProject`` class like the preconditions and postconditions. We also use the ``__init__`` method as a decorator function, and only interact with the class and not any instances. In fact, it is impossible with the standard instantiation model to get an ``operation`` object.

…tives""

bdice

@b-butler I was curious what this PR did, so I gave it a very quick read-through. I recognize it's early stage, but thought that some design feedback might be helpful.

tests/test_project.py

flow/project.py

doc/api.rst

The overwriting of __init__ does not work

This is necessary since reverting to using an OperationRegister object not class

codecov · 2021-04-21T14:24:38Z

Codecov Report

Merging #502 (895fa20) into master (e56c84b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
- Coverage   74.31%   74.31%   -0.01%     
==========================================
  Files          30       30              
  Lines        2998     3013      +15     
  Branches      575      577       +2     
==========================================
+ Hits         2228     2239      +11     
- Misses        621      626       +5     
+ Partials      149      148       -1

Impacted Files	Coverage Δ
flow/directives.py	`98.03% <ø> (ø)`
flow/environments/incite.py	`88.73% <ø> (ø)`
flow/operations.py	`83.33% <100.00%> (+0.98%)`	⬆️
flow/project.py	`77.69% <100.00%> (-0.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e56c84b...895fa20. Read the comment docs.

This is to make the code coverage tests pass

b-butler · 2021-04-23T14:35:04Z

Here is the documentation PR glotzerlab/signac-docs#128

I also added tests for lines of code that code coverage was failing me for not testing even though I did not write those lines of code, in doing so I fixed a small bug.

bdice

We should also modify the test files like define_test_project.py so that they use a mixture of the old and new syntax, at least until we deprecate/remove @directives.

flow/environments/incite.py

flow/project.py

@Directives

Also removes @Directives from defined FlowProjects for tests

bdice

I have a few smaller requests / questions. I approve after those comments are addressed. @lyrivera @kidrahahjo You're both assigned to this PR, could one of you review? Thanks!

flow/project.py

tests/define_test_project.py

tests/test_project.py

Co-authored-by: Bradley Dice <[email protected]>

bdice · 2021-05-17T19:14:56Z

@b-butler Can you resolve merge conflicts? I think that some nontrivial change may be required.

b-butler · 2021-05-18T16:31:04Z

@bdice Done.

…ith-directives

kidrahahjo

I really like this implementation. These changes look good to me. @b-butler @bdice Is there a need to add documentation for this in signac-docs?

b-butler · 2021-05-21T17:20:28Z

@kidrahahjo there is a docs PR glotzerlab/signac-docs#128

b-butler requested review from a team as code owners April 20, 2021 20:32

b-butler requested review from kidrahahjo and lyrivera and removed request for a team April 20, 2021 20:32

b-butler added 2 commits April 20, 2021 16:33

Update changelog for FlowProject.operation.with_directives.

00eed2e

Move documentation examples to use ``FlowProject.operation.with_direc…

ffa7fec

…tives""

bdice reviewed Apr 20, 2021

View reviewed changes

tests/test_project.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

doc/api.rst Show resolved Hide resolved

b-butler mentioned this pull request Apr 21, 2021

Update documentation to use new operation.with_directives decorator glotzerlab/signac-docs#128

Merged

3 tasks

b-butler added 4 commits April 20, 2021 21:02

Return to using an instance for FlowProject.operation

bacd044

The overwriting of __init__ does not work

Rename FlowProject specific operation to OperationRegister

45862c2

Revert OperationRegister.with_directives to method

88f309f

This is necessary since reverting to using an OperationRegister object not class

Fix test name typo directies -> directives

9f75c91

b-butler requested a review from bdice April 21, 2021 19:48

b-butler added 2 commits April 23, 2021 10:28

Add tests to improve codecoverage

e537d38

This is to make the code coverage tests pass

Fix bug where user groups could overwrite operation names

f3e3ec3

b-butler and others added 3 commits April 23, 2021 10:38

Merge branch 'master' into feature/operation-with-directives

65d8702

Merge branch 'master' into feature/operation-with-directives

a2139dc

Merge branch 'master' into feature/operation-with-directives

20d5711

bdice requested changes May 6, 2021

View reviewed changes

flow/environments/incite.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

bdice and others added 6 commits May 6, 2021 11:34

Add comma.

4e6d722

Use double quotes for consistency.

7831c1d

Remove empty directives line.

112137d

Use dict syntax.

632e6e1

Deprecate @Directives and remove from all but one test

b4bc426

Deprecate @Directives and remove from all but one test

718310d

Also removes @Directives from defined FlowProjects for tests

b-butler requested a review from bdice May 13, 2021 23:59

bdice approved these changes May 14, 2021

View reviewed changes

flow/project.py Outdated Show resolved Hide resolved

flow/project.py Outdated Show resolved Hide resolved

tests/define_test_project.py Outdated Show resolved Hide resolved

tests/test_project.py Outdated Show resolved Hide resolved

bdice assigned b-butler May 14, 2021

bdice added this to the v0.15.0 milestone May 14, 2021

bdice added directives Operation directives enhancement New feature or request labels May 14, 2021

b-butler and others added 2 commits May 14, 2021 11:39

Apply suggestions from code review

5573dde

Co-authored-by: Bradley Dice <[email protected]>

Update changelog

bf690f5

b-butler added 2 commits May 18, 2021 11:12

Merge branch 'master' into feature/operation-with-directives

a856c8d

Remove old tests on invalid operation func signatures

b42b521

bdice added 2 commits May 20, 2021 15:05

Merge remote-tracking branch 'origin/master' into feature/operation-w…

65cb99e

…ith-directives

Update changelog.

895fa20

kidrahahjo approved these changes May 21, 2021

View reviewed changes

b-butler merged commit d3f54fb into master May 21, 2021

b-butler deleted the feature/operation-with-directives branch May 21, 2021 17:22

bdice mentioned this pull request Jun 11, 2021

Move directives decorator to operations.directive #309

Closed

b-butler linked an issue Jun 23, 2021 that may be closed by this pull request

Move directives decorator to operations.directive #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add operation.with_directives decorator #502

Add operation.with_directives decorator #502

b-butler commented Apr 20, 2021 •

edited

Loading

bdice left a comment

codecov bot commented Apr 21, 2021 •

edited

Loading

b-butler commented Apr 23, 2021

bdice left a comment

bdice left a comment

bdice commented May 17, 2021 •

edited

Loading

b-butler commented May 18, 2021

kidrahahjo left a comment

b-butler commented May 21, 2021

Add operation.with_directives decorator #502

Add operation.with_directives decorator #502

Conversation

b-butler commented Apr 20, 2021 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

bdice left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 21, 2021 • edited Loading

Codecov Report

b-butler commented Apr 23, 2021

bdice left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

bdice commented May 17, 2021 • edited Loading

b-butler commented May 18, 2021

kidrahahjo left a comment

Choose a reason for hiding this comment

b-butler commented May 21, 2021

b-butler commented Apr 20, 2021 •

edited

Loading

codecov bot commented Apr 21, 2021 •

edited

Loading

bdice commented May 17, 2021 •

edited

Loading