Watch20

Star402

Fork

You've already forked forgejo-cli

Code Issues 44 Pull requests 8 Releases 11 Packages 2 Wiki Activity

feat!: standardize repository specification across all commands #236

Merged

Cyborusmerged 1 commit from nolan/forgejo-cli:cli-consistency into main 2025-11-17 23:58:52 +01:00

Conversation 2 Commits 1 Files changed 5 +39 -40

nolan commented 2025-11-09 23:26:11 +01:00

Contributor

Copy link

Examples:

fj repo view REPO → fj repo view --repo REPO
fj repo star REPO → fj repo star --repo REPO
fj issue create --repo owner/name "Title"
fj pr create --remote upstream "Title"

All repo-scoped commands now support:

--repo/-r: Override repository (format: owner/name)
--remote/-R: Specify git remote for repository detection

Repository commands now use --repo/-r and --remote/-R flags instead of positional arguments. This provides a consistent interface across all commands (issues, PRs, releases, wiki, actions, and repo operations). Examples: - fj repo view REPO → fj repo view --repo REPO - fj repo star REPO → fj repo star --repo REPO - fj issue create --repo owner/name "Title" - fj pr create --remote upstream "Title" All repo-scoped commands now support: - --repo/-r: Override repository (format: owner/name) - --remote/-R: Specify git remote for repository detection

Cyborus requested changes 2025-11-10 17:57:06 +01:00 Dismissed

Cyborus left a comment

Copy link

Regarding moving the --remote option: I appreciate the contribution, but the code duplication seems substantial. There's a simpler way to do this.

Regarding making repo commands take --repo: I think it's more intuitive for them to be positional arguments. The repo is the main object of the operation, and having a flag named repo seems redundant when "repo" is already part of the command name.

Regarding moving the --remote option: I appreciate the contribution, but the code duplication seems substantial. There's a simpler way to do this. Regarding making repo commands take --repo: I think it's more intuitive for them to be positional arguments. The repo is the main object of the operation, and having a flag named repo seems redundant when "repo" is already part of the command name.

src/issues.rs Outdated

Show outdated Hide outdated

Cyborus commented 2025-11-10 01:56:24 +01:00

Member

Copy link

What's the motivation for moving the --remote flag into each command? Every issue (and PR, release, etc) command accepts the flag, so this causes a lot of duplicate code.

clap has a global option on args that allows setting the flag anywhere in the command. I'd prefer to use that instead to keep duplication down.

It'd look something like

patch

#[derive(Args, Clone, Debug)]
 pub struct IssueCommand {
     /// The local git remote that points to the repo to operate on.
- #[clap(long, short = 'R')]
+ #[clap(long, short = 'R', global = true)]
     remote: Option<String>,
     #[clap(subcommand)]
     command: IssueSubcommand,
 }

This would let something like fj issue view --remote origin work.

What's the motivation for moving the --remote flag into each command? Every issue (and PR, release, etc) command accepts the flag, so this causes a lot of duplicate code. clap has a global option on args that allows setting the flag anywhere in the command. I'd prefer to use that instead to keep duplication down. It'd look something like patch #[derive(Args, Clone, Debug)] pub struct IssueCommand { /// The local git remote that points to the repo to operate on. - #[clap(long, short = 'R')] + #[clap(long, short = 'R', global = true)] remote: Option\<String\>, #[clap(subcommand)] command: IssueSubcommand, } This would let something like fj issue view --remote origin work.

src/prs.rs Outdated

Show outdated Hide outdated

Cyborus commented 2025-11-10 01:59:06 +01:00

Member

Copy link

I think specify the short char is unnecessary, it will be inferred from the first letter of repo. I had specified R on remote so it wouldn't conflict.

src/repo.rs Outdated

Show outdated Hide outdated

| | | | @ -347,2 +347,4 @@ | | | | | | /// Fork a repository onto your account | | | | | | Fork { | | | | | | /// Repository to fork | | | | | | #[clap(long, short = 'r')] |

Cyborus commented 2025-11-10 02:06:51 +01:00

Member

Copy link

I'm not much a fan of this change. I prefer the main target of a command to be taken in as a positional argument.

src/repo.rs Outdated

Show outdated Hide outdated

| | | | @ -413,0 +423,4 @@ | | | | | | Star { | | | | | | /// The repo to star | | | | | | #[clap(long, short = 'r')] | | | | | | repo: Option<RepoArg>, |

Cyborus commented 2025-11-10 02:08:40 +01:00

Member

Copy link

I do think making this repo field optional makes sense, to allow starring the current repo. Though again I'd prefer it be positional.

src/repo.rs Outdated

Show outdated Hide outdated

| | | | @ -419,0 +443,4 @@ | | | | | | Delete { | | | | | | /// The repo to delete | | | | | | #[clap(long, short = 'r')] | | | | | | repo: Option<RepoArg>, |

Cyborus commented 2025-11-10 02:10:11 +01:00

Member

Copy link

...though I'm hesitant to make it optional here. Deleting a repository is a very serious thing, I'd prefer to make the user write out the full name of what they want to delete, to make mistakes less likely.

src/repo.rs Outdated

Show outdated Hide outdated

| | | | @ -519,3 +554,3 @@ | | | | | | let repo = RepoInfo::get_current(host_name, Some(&repo), None, &keys)?; | | | | | | let api = keys.get_api(repo.host_url()).await?; | | | | | | let name = repo.name().unwrap(); | | | | | | let name = repo.name().ok_or_eyre("couldn't get repo name, please specify")?; |

Cyborus commented 2025-11-10 02:12:06 +01:00

Member

Copy link

This would be better as expect instead of ok_or_eyre, since the repo arg isn't optional on Clone, so the repo.name() shouldn't ever fail. If it does, it's a bug, not something the user did incorrectly.

nolan force-pushed cli-consistency from e7741311a8

Some checks failed

ci/woodpecker/pr/check Pipeline failed

Details

to b0dedee36e

Some checks failed

ci/woodpecker/pr/check Pipeline failed

Details

2025-11-17 18:27:47 +01:00 Compare

nolan commented 2025-11-17 18:28:18 +01:00

Author

Contributor

Copy link

Thanks for the feedback, didn't know about global=true. I went ahead and fixed that, reverted the requested arguments to be positional, and added global=true in a few other places. I hope this addresses everything--please let me know if I missed something.

Cyborus requested changes 2025-11-17 19:20:59 +01:00 Dismissed

Cyborus left a comment

Copy link

Looks good! If you could make those two changes, and cargo fmt them, I'll happily merge this

src/prs.rs Outdated

Show outdated Hide outdated

Cyborus commented 2025-11-17 19:07:12 +01:00

Member

Copy link

Most PR commands take the repo as part of the PR name, i.e. fj pr view forgejo-contrib/forgejo-cli#236, matching how they're referenced in issues & comments. Search and Create are the only ones that acknowledge the --repo flag. Since they're the exceptions, I think it makes more sense to define the flags in the subcommand for these.

nolan force-pushed cli-consistency from b0dedee36e

Some checks failed

ci/woodpecker/pr/check Pipeline failed

Details

to 89bd8c8a12 2025-11-17 23:29:40 +01:00 Compare

Cyborus approved these changes 2025-11-17 23:50:44 +01:00

Cyborus left a comment

Copy link

Thank you! :)

Cyborus merged commit dfad912c07 into main 2025-11-17 23:58:52 +01:00

Cyborus referenced this pull request from a commit 2025-11-17 23:58:54 +01:00 Merge pull request 'feat!: standardize repository specification across all commands' (#236) from nolan/forgejo-cli:cli-consistency into main

Cyborus added this to the v0.4.0 milestone 2025-12-01 17:03:48 +01:00

Reviewers

No reviewers

Cyborus

Labels

Clear labels[ Kind/Breaking Breaking change that won't be backward compatible

](#)[ Kind/Bug Something is not working

](#)[ Kind/Design Discussion about UI/UX design

](#)[ Kind/Documentation Documentation changes

](#)[ Kind/Enhancement Improve existing functionality

](#)[ Kind/Feature New functionality

](#)[ Kind/Security This is security issue

](#)[ Kind/Testing Issue or pull request related to testing

](#)[ Kind/Upstream This is an issue with upstream software (Forgejo) that is probably not our fault

](#)

[ Priority

Critical The priority is critical

](#)[ Priority

High The priority is high

](#)[ Priority

Low The priority is low

](#)[ Priority

Medium The priority is medium

](#)

[ Reviewed

Confirmed Issue has been confirmed

](#)[ Reviewed

Duplicate This issue or pull request already exists

](#)[ Reviewed

Invalid Invalid issue

](#)[ Reviewed

Won't Fix This issue won't be fixed

](#)

[ Status

Abandoned Somebody has started to work on this but abandoned work

](#)[ Status

Blocked Something is blocking this issue or pull request

](#)[ Status

Need More Info Feedback is required to reproduce issue or to continue work

](#)

[ Suspicious

](#)

No labels Kind/Breaking Kind/Bug Kind/Design Kind/Documentation Kind/Enhancement Kind/Feature Kind/Security Kind/Testing Kind/Upstream [ Priority

Critical ](/forgejo-contrib/forgejo-cli/pulls?labels=173012) [ Priority

High ](/forgejo-contrib/forgejo-cli/pulls?labels=173013) [ Priority

Low ](/forgejo-contrib/forgejo-cli/pulls?labels=173015) [ Priority

Medium ](/forgejo-contrib/forgejo-cli/pulls?labels=173014) [ Reviewed

Confirmed ](/forgejo-contrib/forgejo-cli/pulls?labels=173007) [ Reviewed

Duplicate ](/forgejo-contrib/forgejo-cli/pulls?labels=173005) [ Reviewed

Invalid ](/forgejo-contrib/forgejo-cli/pulls?labels=173006) [ Reviewed

Won't Fix ](/forgejo-contrib/forgejo-cli/pulls?labels=173008) [ Status

Abandoned ](/forgejo-contrib/forgejo-cli/pulls?labels=173011) [ Status

Blocked ](/forgejo-contrib/forgejo-cli/pulls?labels=173010) [ Status

Need More Info ](/forgejo-contrib/forgejo-cli/pulls?labels=173009) Suspicious

Milestone

Clear milestone

No items

No milestone v0.4.0

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo-contrib/forgejo-cli!236

WritePreview

Loading…

Add table

CancelOK

Add a link

Url Description Hint: With a URL in your clipboard, you can paste directly into the editor to create a link.

CancelOK

CancelSave

Reference in a new issue

Repository

forgejo-contrib/forgejo-cli

Title

Body

Create issue

No description provided.

Delete branch "nolan/forgejo-cli:cli-consistency"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

No Yes