-
Notifications
You must be signed in to change notification settings - Fork 2
allow multiple hashtags #13
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,7 +111,7 @@ def get_parser(): | |
| "--hashtag", | ||
| dest="hashtag", | ||
| 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", | ||
| ) | ||
|
|
||
| update.add_argument( | ||
|
|
@@ -282,7 +282,7 @@ def deploy_slack(webhook, message): | |
| ) | ||
|
|
||
|
|
||
| def deploy_bluesky(client, entry, keys, hashtag): | ||
| def deploy_bluesky(client, entry, keys, hashtags): | ||
| """ | ||
| Deploy to bluesky. We add the job link separately. | ||
| """ | ||
|
|
@@ -291,11 +291,12 @@ def deploy_bluesky(client, entry, keys, hashtag): | |
| # Prepare the post, but without the url | ||
| post = prepare_post(entry, keys, without_url=True) | ||
| choice = random.choice(icons) | ||
| message = f"New {hashtag} Job! {choice}\n" | ||
| print(message) | ||
|
|
||
| # Add the text to the textbuilder | ||
| tb.text(message) | ||
| # seems like a clumsy way to build such a message... | ||
| tb.text("New ") | ||
| for hashtag in hashtags: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I did a slightly cleaner replacement in |
||
| tb.tag(hashtag, hashtag.lstrip("#")).text(" ") | ||
| tb.text(f"Job! {choice}\n") | ||
| anchor=entry.get('title') or entry.get('name') or entry.get('url') | ||
| tb.link(anchor, entry.get('url')) | ||
| tb.text("\n" + post) | ||
|
|
@@ -370,6 +371,9 @@ def help(return_code=0): | |
| # Parse keys into list | ||
| keys = [x for x in args.keys.split(",") if x] | ||
|
|
||
| # Parse hashtags into list | ||
| hashtags = [x for x in args.hashtag.split(",") if x] | ||
|
|
||
| # Find new posts in updated | ||
| previous = set() | ||
| missing_count = 0 | ||
|
|
@@ -410,8 +414,8 @@ def help(return_code=0): | |
| # Prepare the post | ||
| post = prepare_post(entry, keys) | ||
| choice = random.choice(icons) | ||
| message = f"New {args.hashtag} Job! {choice}: {post}" | ||
| newline_message = f"New {args.hashtag} Job! {choice}\n{post}" | ||
| message = f"New {(" ".join(hashtags))} Job! {choice}: {post}" | ||
| newline_message = f"New {(" ".join(hashtags))} Job! {choice}\n{post}" | ||
| print(message) | ||
|
|
||
| # Convert dates, etc. back to string | ||
|
|
@@ -435,7 +439,7 @@ def help(return_code=0): | |
| deploy_twitter(twitter_client, newline_message) | ||
|
|
||
| if args.deploy_bluesky and bluesky_client: | ||
| deploy_bluesky(bluesky_client, entry, keys, args.hashtag) | ||
| deploy_bluesky(bluesky_client, entry, keys, hashtags) | ||
|
|
||
| # If we are instructed to deploy to mastodon and have a client | ||
| if args.deploy_mastodon and mastodon_client: | ||
|
|
||
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.
For args that are intended to become lists, you can use
action="append"and then have the default be None, and do something like: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:So I would keep the argument dest as just
hashtagto match the user interaction better, and then parseargs.hashtagas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good idea. I like this method better. I'll make that change soon.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the default, what you'd want to do is set to
None, and then:Agree on the second point.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.