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

emitter: add "streaming brief" feature #166

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Conversation

tigarmo
Copy link
Collaborator

@tigarmo tigarmo commented Aug 9, 2023

This commit adds a new feature to the Emitter, tentatively called "streaming brief". It is initially disabled, and can be enabled in the Emitter's init() call.

Once enabled, the feature will leverage the "multi-action" nature of progress() calls to "stream", in a single line, info-level log messages and text written to the open_stream()'s pipe. For example, the following sequence of calls in BRIEF mode:

emit.progress("Starting stage 1", permanent=False) ...
log.info("Doing first step")
...
log.info("Doing second step")
...
emit.progress("Finished stage 1", permanent=True)

... will cause the two 'info' messages to "stream" on the terminal, prefixed by "Starting stage 1 ::" to indicate that they are related to that progress message. All these three messages are ephemeral which means that in the end only the final progress() call will be visible on the terminal.

Fixes #165

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@tigarmo tigarmo force-pushed the CRAFT-1951-stream-log-brief branch 2 times, most recently from e9cf33a to 289925b Compare August 9, 2023 18:03
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #166 (3ad948e) into main (659cac9) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   94.40%   94.54%   +0.13%     
==========================================
  Files           7        7              
  Lines        1019     1044      +25     
  Branches      187      192       +5     
==========================================
+ Hits          962      987      +25     
  Misses         53       53              
  Partials        4        4              
Files Changed Coverage Δ
craft_cli/messages.py 100.00% <100.00%> (ø)
craft_cli/printer.py 100.00% <100.00%> (ø)

@tigarmo tigarmo force-pushed the CRAFT-1951-stream-log-brief branch 4 times, most recently from 5f99e52 to 5fe64cf Compare August 9, 2023 18:34
This commit adds a new feature to the Emitter, tentatively called
"streaming brief". It is initially disabled, and can be enabled in
the Emitter's init() call.

Once enabled, the feature will leverage the "multi-action" nature of
progress() calls to "stream", in a single line, info-level log messages
and text written to the open_stream()'s pipe when in BRIEF mode. For
example, the following sequence of calls (in BRIEF mode):

```
emit.progress("Starting stage 1", permanent=False)
...
log.info("Doing first step")
...
log.info("Doing second step")
...
emit.progress("Finished stage 1", permanent=True)
```

... will cause the two 'info' messages to "stream" on the terminal,
prefixed by "Starting stage 1 ::" to indicate that they are related to
that progress message. All these three messages are ephemeral which
means that in the end only the final progress() call will be visible
on the terminal.

Fixes #165
@tigarmo tigarmo force-pushed the CRAFT-1951-stream-log-brief branch from 5fe64cf to e60af45 Compare August 9, 2023 18:38
@tigarmo tigarmo marked this pull request as ready for review August 9, 2023 18:45
@tigarmo tigarmo changed the title emiter: add "streaming brief" feature emitter: add "streaming brief" feature Aug 9, 2023
Copy link
Contributor

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I don't have any specific changes to request, but I'd like to hear what you think about some of my comments.

craft_cli/messages.py Outdated Show resolved Hide resolved
craft_cli/messages.py Show resolved Hide resolved
craft_cli/messages.py Outdated Show resolved Hide resolved
craft_cli/messages.py Outdated Show resolved Hide resolved
craft_cli/printer.py Outdated Show resolved Hide resolved
examples.py Show resolved Hide resolved
tests/unit/test_messages_integration.py Outdated Show resolved Hide resolved
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

It looks reasonable in a global overview, but it's possible that the changes could result in unexpected behavior in some corner case. Let's keep an eye open and fix later if needed.

craft_cli/messages.py Show resolved Hide resolved
craft_cli/messages.py Outdated Show resolved Hide resolved
craft_cli/messages.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Thanks for humouring my questions!

@tigarmo tigarmo merged commit c7e4492 into main Aug 10, 2023
8 checks passed
@tigarmo tigarmo deleted the CRAFT-1951-stream-log-brief branch August 10, 2023 21:52
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.

Support "streaming" INFO log messages in BRIEF mode
3 participants