cmd/bsky-webhook: add multiple words, case insenstivity and word boundary #19
Conversation
Fixes #18 Signed-off-by: Erisa A <erisa@tailscale.com>
| @@ -42,7 +43,10 @@ var ( | |||
| "https://bsky.social"), "bluesky PDS server URL") | |||
| watchWord = flag.String("watch-word", envOr("WATCH_WORD", "tailscale"), | |||
| "the word to watch out for. may be multiple words in future (required)") | |||
|
|
|||
| watchWords = flag.String("watch-words", envOr("WATCH_WORDS", ""), // "word1;word2" | |||
| "the words to watch out for. if watch-word is also specified, they are added") | |||
There was a problem hiding this comment.
Separately: I guess you're adding a new flag rather than extending the existing one to avoid breaking existing use. Should we maybe make them mutually exclusive to make a clean break?
|
|
||
| watchWords = flag.String("watch-words", envOr("WATCH_WORDS", ""), // "word1;word2" | ||
| "the words to watch out for. if watch-word is also specified, they are added") | ||
| delimiter = flag.String("word-delimiter", envOr("WORD_DELIMITER", ";"), |
There was a problem hiding this comment.
Does this really need to be a parameter? That seems like overkill.
If we do want to allow arbitrary substrings rather than "words", I feel like maybe we should define a more robust format, e.g., pick a delimiter and define an escape sequence. Is there a specific use case you have in mind here?
There was a problem hiding this comment.
It's to allow for watching "words" that contain the character in the default delimiter, it's not intended to be changed in most cases. I hear you though
There was a problem hiding this comment.
Another possibility, if you don't wind up going the regexp route, would be to have a flag that lets unclaimed non-flag arguments act as watch words:
var useWatchArgs = flag.Bool("watchword-args", false, "Use non-flag arguments as watch words")
// ...
var watchWords []string
if *useWatchArgs {
watchWords = flag.Args()
} else if flag.NArg() != 0 {
log.Fatalf("extra arguments after command: %q", flag.Args())
} else if *watchWord != "" {
watchWords = []string{*watchWord}
}
or words to that effect.
There was a problem hiding this comment.
That's a good point actually, then the shell would be responsible for separating up "words" right?
There was a problem hiding this comment.
Yeah, the caller can do any quoting they need to to preserve whitespace, and otherwise it'll get split up before exec.
| @@ -51,6 +55,10 @@ var ( | |||
| "the Tailscale hostname the server should advertise (if empty, runs locally)") | |||
| tsStateDir = flag.String("ts-state-dir", envOr("TS_STATE_DIR", ""), | |||
| "the Tailscale state directory path (optional)") | |||
| caseSensitive = flag.Bool("case-sensitive-words", hasEnv("CASE_SENSITIVE_WORDS"), | |||
| "make watch words case-sensitive") | |||
| enforceWordBoundary = flag.Bool("enforce-word-boundary", hasEnv("ENFORCE_WORD_BOUNDARY"), | |||
There was a problem hiding this comment.
Hmm.
What do you think instead about having the word matches be regexps? Then the user can specify case (in)sensitivity directly, and the choice to enforce a word boundary can be encoded in the expression.
That would also give us a way to express multiple matches, since the RE2 syntax already has a way to express alternation.
There was a problem hiding this comment.
I like it, the only thing I worry is that it gets confusing when you have multiple words to make sure to group them and add (or not) the options for each group
There was a problem hiding this comment.
Especially since I wanted character sensitivity to be disabled by default since that's usually what you want and it's not always obvious unless pointed out that you need to enable it, especially when enabling it requires adding in some characters to your regex
There was a problem hiding this comment.
Ah yeah, so it might still make sense to have a separate flag for "enable case sensitivity". That could still be grafted with a regexp, though, e.g., https://go.dev/play/p/-LQN-fCPhdN
Something like that would let you have the default case-insensitive match without having to explicitly ask for it, yet still give you the ability to control it finely if you want.
Erisa
changed the title
Fixes #18