Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Push: add custom messenger (BC) #17211
Push: add custom messenger (BC) #17211
Changes from 3 commits
2fdd2a5
c5627fc
ea3b20a
b5e2a9e
03f693e
0e22eae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is too simplistic, especially when
title
andmessage
can contain commas (to tabs fortsv
). Do we really need csv and tsv ? I can't see any good use case that can not be solved with a stronger typed representation (like json).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.
Does it hurt? Since users do control the message format this can be handled by crafting the right kind of message. Happy to improve this by doing a better encoding.
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.
Nvm. We can just use the cvswriter for this as well.
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.
Well, I've been in the open-source software business for too long, I guess. I've learned over the long run to respect YAGNI and better leave out things than anticipate anything the people might use at some point in time. It's easy to add these features later anyway if needed, but if nobody uses them, you will carry that baggage with you forever. Ideally, you add some tests for it (especially for corner cases and strange escape rules), and you need some documentation. I became a fanboy of less is more most of the time.
Or let me ask the other way around: Which use case do you think needs CSV, TSV, or title—only for sending out to an endpoint? Couldn't you just do it all yourself when defining the
msg
, even with the proper templating support?In the end, it's your call anyway, as I'm like many other contributors: Here for some time to help, but likely not forever. Just sharing some stuff.
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.
Anybody using the
script
messenger right 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.
I don't think so.
The script provider does not implement
SetStringProvider
(so it wouldn't work anyway as is). I would keep it simplistic and ignore the title.Empty titles will lead to strange CSV and TSV representations (like a leading
,
or leading tab), too. Still, we should allow for empty titles for this kind of integration, as this would be the most natural way to define notitle
and a JSONmsg
when using a custom provider (you can always add atitle:
in thatmsg
that the user defines if you like)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.
Using tsv with
Getting title and message as two separate arguments will not work when $msg (or $title) contains spaces.
So I think that most of the time, one would use
cmdline: 'script "${send}"'
to get the whole input as a single argument to the script. I would always leave out the title for custom providers.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.
this type does not "fit" into the list: json, csv, tsv are formats (so kind of an encoding), "title" and "message" (the default) are more filters.
Wdyt to restrict encoding to "json" and "string" (or "raw") and have some second options as to whether to include the title or not ? (usually if you only use a single service endpoint and you don't need the title, you just don't define it in
events:
...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.
It doesn't, but it allows returning the title instead of the message for simple command line scripts. It doesn't hurt anyway.
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 still don't buy the argument it doesn't hurt anyway, but your mileage may vary :-)
This file was deleted.