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

Fix #228 - better output logic #231

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

Conversation

lars-t-hansen
Copy link
Collaborator

@lars-t-hansen lars-t-hansen commented Jan 13, 2025

This allows all operations to print csv or json (and in the future, eg a binary format) by abstracting the output logic. Generally this is a nice improvement since it separates the output logic completely from the rest of the program logic.

TODO here:

  • more testing on various hardware
  • probably some test cases
  • possibly, at the upper level there should be some attention paid to buffering output and flushing the buffer, not sure yet what stdout does (in Go it is unbuffered)
  • All JSON packets should have an outermost object that can take an expanding array of named fields, this leads to better extensibility over time (only fields are added, the data format is unchanged), cf Error back channel / side channel #233.

@lars-t-hansen
Copy link
Collaborator Author

stdout is line-buffered (but has no flush method). By taking a reference to a stdout object and casting it to an io::Write we also gain the ability to call flush on it, which is what we want.

@lars-t-hansen lars-t-hansen requested a review from bast January 13, 2025 13:46
@lars-t-hansen lars-t-hansen marked this pull request as ready for review January 13, 2025 13:46
@lars-t-hansen lars-t-hansen marked this pull request as draft January 15, 2025 10:00
@lars-t-hansen lars-t-hansen removed the request for review from bast January 15, 2025 10:00
@lars-t-hansen
Copy link
Collaborator Author

Converting this back to draft, I'd like to revisit the JSON representation of slurm data, cf #233.

@lars-t-hansen lars-t-hansen marked this pull request as ready for review January 15, 2025 12:55
@lars-t-hansen lars-t-hansen requested a review from bast January 15, 2025 12:55
@lars-t-hansen
Copy link
Collaborator Author

Rebased and fairly clean, @bast review at will.

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.

1 participant