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

libsubprocess: add extra documentation #6307

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Sep 25, 2024

per comment in #6281 some extra comments could be useful. I did find myself struggling remembering how read vs write fbuf watchers behaved too.

@chu11 chu11 force-pushed the libsubprocess_documentation branch from 81ddabb to d7a1998 Compare September 25, 2024 18:51
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

Problem: There is a minor English error in the fbuf comments.

Fix the English error.
Problem: There were some inconsistent tabbing in libsubprocess code.

Fix up the tabbing.
Problem: Comments on the fbuf read and write watcher are a little
thin.

Add some additional comments to remind readers what the watchers do.
Problem: If output callbacks do not read from their respective
buffers, it can lead to excessive callbacks and code spinning.

Add comment about this in subprocess.h.
@chu11 chu11 force-pushed the libsubprocess_documentation branch from d7a1998 to ee08669 Compare September 27, 2024 05:32
@mergify mergify bot merged commit 64162af into flux-framework:master Sep 27, 2024
33 checks passed
@chu11 chu11 deleted the libsubprocess_documentation branch September 27, 2024 06:45
Copy link

codecov bot commented Dec 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.30%. Comparing base (891dbc4) to head (ee08669).
Report is 536 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6307      +/-   ##
==========================================
- Coverage   83.32%   83.30%   -0.02%     
==========================================
  Files         523      523              
  Lines       86133    86133              
==========================================
- Hits        71770    71757      -13     
- Misses      14363    14376      +13     
Files with missing lines Coverage Δ
src/common/libsubprocess/local.c 82.74% <ø> (ø)

... and 8 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants