Skip to content

Add a new flag dput --comment-extra to add text to the commit message#108

Open
refi64 wants to merge 1 commit intomainfrom
wip/refi64/dput-comment-extra
Open

Add a new flag dput --comment-extra to add text to the commit message#108
refi64 wants to merge 1 commit intomainfrom
wip/refi64/dput-comment-extra

Conversation

@refi64
Copy link
Copy Markdown
Collaborator

@refi64 refi64 commented Apr 30, 2026

If non-empty, it gets appended after the .dsc filename (the current message).

Fixes #107.

@refi64 refi64 requested a review from emanueleaina April 30, 2026 15:32
@refi64
Copy link
Copy Markdown
Collaborator Author

refi64 commented Apr 30, 2026

@emanueleaina I'm not sure if the format DSC\n\nEXTRA is what you envisioned exactly? It would be conceptually cool to just pass an entire custom template, but that would also obviously be a lot more effort; my concern is more as to whether or not shuffling the EXTRA bit into a separate line is Good Enough.

emanueleaina
emanueleaina previously approved these changes May 1, 2026
Copy link
Copy Markdown
Collaborator

@emanueleaina emanueleaina left a comment

Choose a reason for hiding this comment

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

Ooooh, this is definitely what I was looking for, thank you!

Comment thread obo-core/src/upload.rs Outdated
Comment on lines +265 to +268
let commit_message = comment_extra.map_or_else(
|| dsc_filename.to_owned(),
|extra| format!("{dsc_filename}\n\n{extra}"),
);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

non-blocking[bikeshedding]: when not chaining I find match to be more straightforward than map_or_else:

Suggested change
let commit_message = comment_extra.map_or_else(
|| dsc_filename.to_owned(),
|extra| format!("{dsc_filename}\n\n{extra}"),
);
let commit_message = match comment_extra {
None => dsc_filename.to_owned(),
Some(extra) => format!("{dsc_filename}\n\n{extra}"),
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah agreed; Or even just an if let Some(message) = :) Many ways to skin that cat, but the more functional combinator doesn't seem to add value here

Comment thread obo-core/src/actions.rs Outdated
#[clap(long, flag_supporting_explicit_value())]
pub rebuild_if_unchanged: bool,
#[clap(long, default_value = "")]
pub comment_extra: String,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub comment_extra: String,
pub comment_extra: Option<String>,

A default value of empty which is then meant to mean unset is odd.

Also bikeshedding on the name why "--comment-extra" ? What's extra about it? Also seems more obvious to use -m, --message similar to e.g. osc commit (which is what we're doing in the end) or things like git

Copy link
Copy Markdown
Collaborator Author

@refi64 refi64 May 1, 2026

Choose a reason for hiding this comment

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

A default value of empty which is then meant to mean unset is odd.

It's intentional, since when using the runner there's no way to conditionally pass an argument, so if you want to optionally give a value the only option is to do --arg=$VALUE anyway and let the empty string mean "no". This is also why branch_to defaults to the empty string just a few lines above.

Also bikeshedding on the name why "--comment-extra" ? What's extra about it? Also seems more obvious to use -m, --message similar to e.g. osc commit (which is what we're doing in the end) or things like git

"extra" since it appends to it, "comment" because I thought OBS called it "comments" and it was previously incorrect of me to use "message" (I had not considered that osc itself uses "message", oops).

Changed now to --message.

Comment thread obo-core/src/upload.rs Outdated

let commit_message = comment_extra.map_or_else(
|| dsc_filename.to_owned(),
|extra| format!("{dsc_filename}\n\n{extra}"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why force the dsc filename to be in the commit message? (also shows why your naming is strange as you go from an extra_comment to a commit_message :p ).

The dsc filename is a good default; But if the user specifies a message they can decide on whether that's useful for them or whether they just want to put in say " <gitlab link/commit/branch/snake>"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was mostly because, if you wanted to exclusively append, then getting the dsc basename yourself in the prior steps is mildly annoying, and OBS shows all lines of the message on the history page anyway. That being said I guess the dsc filename was probably derived dynamically anyway, so in that case it's a pretty easy change?

Updated the code to just always replace the message instead.

Comment thread obo-core/src/upload.rs Outdated
Comment on lines +265 to +268
let commit_message = comment_extra.map_or_else(
|| dsc_filename.to_owned(),
|extra| format!("{dsc_filename}\n\n{extra}"),
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah agreed; Or even just an if let Some(message) = :) Many ways to skin that cat, but the more functional combinator doesn't seem to add value here

@refi64 refi64 force-pushed the wip/refi64/dput-comment-extra branch from 3899e62 to 59e010b Compare May 1, 2026 15:16
If non-empty, it gets appended after the .dsc filename (the current
message).

Fixes #107.
@refi64 refi64 force-pushed the wip/refi64/dput-comment-extra branch from 59e010b to 172cbb2 Compare May 1, 2026 15:19
Comment thread obo-core/src/actions.rs
Comment thread obo-core/src/actions.rs
#[clap(long, flag_supporting_explicit_value())]
pub rebuild_if_unchanged: bool,
#[clap(long, default_value = "")]
pub message: String,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

your commit message/PR title still says comment-extra

Comment thread obo-core/src/actions.rs
Copy link
Copy Markdown
Contributor

@sjoerdsimons sjoerdsimons left a comment

Choose a reason for hiding this comment

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

Just needs the commit message/merge title changing now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Make dput accept some metadata to be attached to the upload

3 participants