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

[kuksa-client] Show subscription events in client. #648

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Sep 4, 2023

  • Show subscription events in client instead of writing them to a file (and suggesting the user tails them manually).

- Show subscription events in client instead of writing them to
  a file (and suggesting the user tails them manually).
@argerus argerus force-pushed the feature/kuksa_client_subscriptions branch from caabbd2 to 13a5c40 Compare September 5, 2023 07:50
@erikbosch
Copy link
Contributor

Looks good to me.

What we possibly could discuss, not necessarily part of this PR, is if we would like to align output format for subscribe and getValue. Today for subscribe you always get a list including "entry" and "fields", but you do not get that for getValue which strictly may not be needed as you know from the command if it is "value" or something else. On the other hand, if we assume that output not will be parsed it does not matter. Also, we do not document how output will look like for subscribe, also a possible future improvement. But nothing that needs to be done for this PR I think.

@SebastianSchildt
Copy link
Contributor

The tail CAN be annoying sometimes, but how does this Pr influence the ability to "keep working" while several subscriptions running? Maybe this should be more like an option?

@argerus
Copy link
Contributor Author

argerus commented Sep 5, 2023

The tail CAN be annoying sometimes, but how does this Pr influence the ability to "keep working" while several subscriptions running? Maybe this should be more like an option?

Good point. I just got annoyed by this indirection, but I guess it can be feature in some cases.

I added an option to enable old behaviour.

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

Works. Nice little quality-of-life-improvement for some use cases without breaking others 🥳

@SebastianSchildt SebastianSchildt merged commit c9cddaf into eclipse:master Sep 6, 2023
5 checks passed
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.

3 participants