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

Documentation for hooks core #154

Merged
merged 73 commits into from
Mar 21, 2022
Merged

Documentation for hooks core #154

merged 73 commits into from
Mar 21, 2022

Conversation

klywang
Copy link
Contributor

@klywang klywang commented Aug 24, 2021 •
edited
Loading

Description

Documentation for signac hooks core (issue 508 in signac-flow).

Motivation and Context

Documentation for new hooks feature.

Checklist:

@klywang klywang requested a review from a team as a code owner August 24, 2021 18:30
@klywang klywang requested a review from a team as a code owner August 24, 2021 18:30
@klywang klywang self-assigned this Aug 24, 2021
@klywang klywang requested review from b-butler and Charlottez112 and removed request for a team August 24, 2021 18:30
@klywang
Copy link
Contributor Author

klywang commented Aug 24, 2021

How do I turn this into a draft?

@klywang klywang changed the title Added some sections about hooks. Documentation for hooks core Aug 24, 2021
@csadorf
Copy link
Contributor

csadorf commented Aug 25, 2021

How do I turn this into a draft?

@klywang It's right under the "Reviewers" section in the top right, see also: https://github.blog/changelog/2020-04-08-convert-pull-request-to-draft/

@klywang klywang marked this pull request as draft August 25, 2021 20:46
@klywang klywang marked this pull request as ready for review August 31, 2021 14:09
Charlottez112
Charlottez112 approved these changes Aug 31, 2021
View reviewed changes
Copy link
Contributor

@Charlottez112 Charlottez112 left a comment

Choose a reason for hiding this comment

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

Thank you for adding the documentation! Very clear and intuitive

b-butler
b-butler previously requested changes Aug 31, 2021
View reviewed changes
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

I left some preliminary comments to address.

docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
klywang and others added 6 commits September 14, 2021 12:53
@klywang @b-butler
Remove redundant information
a2af838 
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@klywang @b-butler
More specific/descriptive wording about hooks.
25dd610 
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@klywang @b-butler
Apply suggestions from code review
130ed25 
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@klywang
Updated decorator names.
c2a2086
@klywang
Wrong reference.
1fc1150
@klywang @b-butler
Update docs/source/hooks.rst
4a19233 
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@cbkerr
Copy link
Member

cbkerr commented Mar 12, 2022

I fixed the last example, so this is ready for a final review @klywang @bdice.

All example code now works when using flow from the master branch.

@cbkerr cbkerr requested review from bdice and atravitz March 12, 2022 18:43
@cbkerr cbkerr requested a review from b-butler March 12, 2022 19:11
bdice
bdice requested changes Mar 12, 2022
View reviewed changes
Copy link
Member

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Feedback attached. Thanks for working on this @cbkerr!

docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
cbkerr and others added 5 commits March 12, 2022 15:02
@cbkerr @bdice
Apply settled suggestions from code review
758661f 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
Update docs/source/hooks.rst
e425d28 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
Update docs/source/hooks.rst
a9b80ec 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
Assign hook to list rather than appending
6f0b548 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
Make it clear that install hooks returns project
213ac29 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr cbkerr requested a review from bdice March 12, 2022 20:40
bdice
bdice requested changes Mar 13, 2022
View reviewed changes
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
cbkerr and others added 3 commits March 13, 2022 13:30
@cbkerr @bdice
Apply suggestions from code review
0484b9e 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
shorten time logging example
ec9c45b 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr @bdice
Make custom hook install take project as arg, not constructor
2edd936 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@cbkerr cbkerr requested a review from bdice March 13, 2022 18:45
@cbkerr cbkerr dismissed their stale review March 13, 2022 18:48

Very old review

bdice
bdice approved these changes Mar 13, 2022
View reviewed changes
docs/source/hooks.rst Outdated Show resolved Hide resolved
@cbkerr @bdice
Update docs/source/hooks.rst
03df8e0 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
b-butler
b-butler requested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

A few small comments to address, and then I will approve.

docs/source/hooks.rst Outdated Show resolved Hide resolved
docs/source/hooks.rst Outdated Show resolved Hide resolved
@cbkerr @b-butler
Remove superfluous comments
ad31054 
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@cbkerr cbkerr requested a review from b-butler March 21, 2022 17:47
@b-butler b-butler merged commit 5ddd462 into master Mar 21, 2022
@b-butler b-butler deleted the feature/hooks-core branch March 21, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbkerr cbkerr cbkerr left review comments

@bdice bdice bdice approved these changes

@b-butler b-butler b-butler approved these changes

@Charlottez112 Charlottez112 Awaiting requested review from Charlottez112

@atravitz atravitz Awaiting requested review from atravitz

Assignees

@cbkerr cbkerr

@klywang klywang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@klywang @csadorf @cbkerr @bdice @atravitz @b-butler @Charlottez112