-
Notifications
You must be signed in to change notification settings - Fork 32
feat(read-only): add --only=retained to only read retained message #193
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
|
Out of curiosity: retained messages are a relatively quick response. So if there is no retained message, waiting for a non-retained message to abort the process might take a while? |
src/read_one.rs
Outdated
| match (only_retained, ignore_retained, publish.retain) { | ||
| (true, true, _) => unreachable!(), | ||
| (true, false, false) => std::process::exit(1), | ||
| (false, true, true) => continue, | ||
| _ => (), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could get more clear with an enum on the CLI?
Just thinking out loud: --retained=ignore and --retained=only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds nice, I will look into this tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spontaneous thought: maybe the flag shouldn't be called --retained=value and more like --only=retained and --only=non-retained or --message-kind=<value> or something like that.
Is there a good name for non-retained messages? Maybe --only=live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe --only=now for only non-retained messages where its clear that they happened just now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--only=new could also be good.
I think --only=… is better than --retained=… as its more clear what's happening. Mainly not sure about the value for the non-retained ones.
Guess the current options seem to be
- live
- now
- new
Any thoughts here? What's the best one to understand from just reading some random script?
bla=$(mqttui read-one --only=live "$topic")
bla=$(mqttui read-one --only=now "$topic")
bla=$(mqttui read-one --only=new "$topic")
Yep, I found that as well, but I don't see a way around it. Introducing timeouts in the codebase just seems wrong to me, TBH. Callers can still do their own timeouts, right? (I am using mqttui in test scripts where this usecase came up, having a timeout there is a no-brainer for me). |
|
Documenting this behavior when running |
|
@matthiasbeyer are you still interested on working on it? If it takes longer, that's fine. If you don't have the time, I can continue on this when I have some time. So see this more of a status check and not as a 'hurry up'. 😇 |
|
I am still interested, yes, but I fail to find the time right now. Feel free to take my patches or discard them, no hard feelings either way! 👍 |
|
For everyone looking in here, I think the code itself is in a good state, mainly unsure about the naming. See #193 (comment) |
I found this helpful for a usecase I have, tell me what you think!?