Conversation
this changeset does two things: 1) replaces the `hashtag` option with `hashtags` that takes a comma separated list of hashtags to use 2) changes the way hashtags are prepared in the BlueSky deployment. Previously, those tags were just static text and were not linkified by bsky in the text of the post Signed-off-by: Chad Dougherty <crd477@icloud.com>
Signed-off-by: Chad Dougherty <crd477@icloud.com>
|
You can see some sample output at this throwaway bsky account. |
|
If you'd rather merge this change into its own tag or branch, that's just as good for me. It may even be better that way. |
| dest="hashtags", | ||
| default="#RSEng", | ||
| help="A hashtag (starting with #) to include in the post, defaults to #RSEng", | ||
| help="A comma separated list of hashtags (starting with #) to include in the post, defaults to #RSEng", |
There was a problem hiding this comment.
For args that are intended to become lists, you can use action="append" and then have the default be None, and do something like:
hashtags = args.hashtags or []To ensure you get an empty list. The default can also be ["RSEng"] as a single item to reproduce what you have now. Then on the command line append works like this:
find-updates.py --hashtag first --hashtag secondSo I would keep the argument dest as just hashtag to match the user interaction better, and then parse args.hashtag as a list. Nit - I would remove the # since it can be parsed as a comment. But if you quote it, probably OK too. Do what you think is best, and just document it for the developer user.
There was a problem hiding this comment.
Ah, good idea. I like this method better. I'll make that change soon.
There was a problem hiding this comment.
OK, I've partially addressed this by returning the command-line argument to its original name. It can still accept a comma-separated list of hashtags though.
I considered the suggestion to supply it as a list in the first place, but there are two reasons that I'd rather not do that and just keep it as a string:
-
the append method makes it more awkward to supply a default value because the default value would still be included in the list. So, for example,
#RSEngwould always be included even if what you really wanted was#one,#two,#threeexclusively. The default option could be supplied later in main, but that feels weird to me when there's already a nice way to supply a default in argparse. -
changing it to a list would require a change to the configuration of existing workflows, and if we agree that we don't want to change the command-line argument for that reason, perhaps we shouldn't change the type of
hashtageither. I'd prefer to keep it as a comma-separated string as is already done forkeys. I think it's less error-prone and easier to supply in the yaml.
There was a problem hiding this comment.
For the default, what you'd want to do is set to None, and then:
args.hashtag = args.hashtag or ["#RSEng"]Agree on the second point.
There was a problem hiding this comment.
For the default, what you'd want to do is set to
None, and then:args.hashtag = args.hashtag or ["#RSEng"]
I'm pretty sure I understand what you mean, but I'd rather not do it this way if it's OK with you. Are you still willing to merge it without this change?
If so, I'll run a few more tests and point you to the results.
There was a problem hiding this comment.
Oh no, I'm good with what you have - I'm just showing for the future how I'd handle the issue you pointed out.
| tb.text(message) | ||
| # seems like a clumsy way to build such a message... | ||
| tb.text("New") | ||
| for hashtag in hashtags: |
There was a problem hiding this comment.
You can do list comprehension instead. And I think tb.text() is going to need to be the entire message? I'm not sure you need "New" that is probably from an example. I would remove that, then do:
# Assume a mixture or erroneous
tags = ["one", "#two", "#three"]
# Clean (normalize) all tags of #
tags = [x.replace('#', "") for x in tags]
# Add back #
tags = " ".join([f"#{tag}" for tag in tags])
message = f"New {tags} Job! {choice}\n"
New #one #two #three Job! 💼You can technically combine the two cleanup and add back into one comprehension, but I'd choose this one because it's easier to read.
There was a problem hiding this comment.
OK, I did a slightly cleaner replacement in main() where the static "dead" string of tags is supplied, and slightly improved the form in deploy_bluesky(). I'd prefer to not use a list comprehension there because I think it would be less readable.
The reason you see what looks like a bare "New" there is because it's building up the bsky post piecemeal, so "New " + linkified tags + "Job!...".
doing so allows existing configurations of this workflow to continue working while still allowing a comma-separated list of hashtags to be supplied partially addresses feedback by @vsoch in the PR Signed-off-by: Chad Dougherty <crd477@icloud.com>
|
OK, I ran three new tests and you can see the bsky output here: https://bsky.app/profile/jpt-dougherty477.bsky.social Please let me know if you feel like it's OK to go, and thanks for your help as always. |
this changeset does two things:
replaces the
hashtagoption withhashtagsthat takesa comma separated list of hashtags to use
changes the way hashtags are prepared in the BlueSky
deployment. Previously, those tags were just static text and
were not linkified by bsky in the text of the post