Skip to content
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

Allow the specification of alt-text on images when using CLI. #247

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PeterGrace
Copy link

This PR adds the ability to specify alt-text on images to help give descriptive values to them. It is written so that the alt-text input is a one-to-one map of strings based on the image filename input.

Let me know what I need to do to help get this approved and merged! I'm not sure what level of documentation is required since I assume bsky-cli is a proof-of-implementation tool, however it works just fine for me for what I need it to do so figured I'd improve it!

@sugyan
Copy link
Owner

sugyan commented Nov 15, 2024

Thank you for the pull-request!
Please check the format first with cargo fmt-no-gen --check.

It is curious that images is plural and alt_text is singular, even though both are Vec. Shall we change it to image to align them?

I see that it also takes into account cases where the number of images does not match, such as --image img1.png --alt-text alt1 --image img2.png. If there is no corresponding idx alt_text, the file name is still used by default. (In this example, the alt text for the second image is “img2.png”.)
Conversely, I would like to consider the case where we want to specify only the second. If I do --image img1.png --alt-text “” --image img2.png -alt-text alt2, I want the alt of the first image to be “img1.png “.

For example, how about the following?

let alt = match args.alt_text.get(idx).filter(|s| !s.is_empty()) {
    ...
};

@sugyan sugyan self-requested a review November 15, 2024 14:35
@@ -87,6 +87,9 @@ pub struct CreatePostArgs {
/// Images to embed
#[arg(short, long)]
pub(crate) images: Vec<PathBuf>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is OK to change this to image.

images.push(
api::app::bsky::embed::images::ImageData {
alt: image
let alt= match args.alt_text.get(idx) {
Copy link
Owner

@sugyan sugyan Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding .filter(|s| !s.is_empty()) or something like that, since an empty string will be assumed to be unspecified and the default filename will be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants