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

Issue edit #245

Merged
merged 11 commits into from
Nov 29, 2018
Merged

Issue edit #245

merged 11 commits into from
Nov 29, 2018

Conversation

claytonrcarter
Copy link
Collaborator

See discussion in #76. I skipped the title and edit flags, and removed the shorthand for unlabel and unassign.

This is fully functional, but before it goes further we should confirm the naming of the flags, and specifically the assign & unassign flags. They pair nicely, of course, and I like how they feel in use but they're inconsistent with other commands: lab issue create uses --assignees (even though it only takes one) and lab mr create uses --assignee. If I change this to use assignee, then unassign feels out of place, but changing unassign to something like remove-assignee feels overly verbose. Feedback or opinions would be helpful.

$ ./lab issue edit -h
Edit or update an issue

Usage:
  lab issue edit [remote] <id> [flags]

Aliases:
  edit, update

Examples:
lab issue edit <id>                                # update issue via $EDITOR
lab issue update <id>                              # same as above
lab issue edit <id> -m "new title"                 # update title
lab issue edit <id> -m "new title" -m "new desc"   # update title & description
lab issue edit <id> -l newlabel --unlabel oldlabel # relabel issue

Flags:
  -a, --assign strings     Add an assignee by username
  -h, --help               help for edit
  -l, --label strings      Add the given label(s) to the issue
  -m, --message strings    Use the given <msg>; multiple -m are concatenated as separate paragraphs
      --unassign strings   Remove an assigne by username
      --unlabel strings    Remove the given label(s) from the issue

@codecov
Copy link

codecov bot commented Nov 14, 2018

Codecov Report

Merging #245 into master will increase coverage by 1.02%.
The diff coverage is 84.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   68.22%   69.25%   +1.02%     
==========================================
  Files          42       43       +1     
  Lines        2219     2387     +168     
==========================================
+ Hits         1514     1653     +139     
- Misses        550      565      +15     
- Partials      155      169      +14
Impacted Files Coverage Δ
internal/gitlab/gitlab.go 42.63% <50%> (+0.15%) ⬆️
cmd/issue_edit.go 86.71% <86.71%> (ø)
cmd/root.go 72.93% <0%> (-0.66%) ⬇️
cmd/issue_list.go 93.93% <0%> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ce4cf...66180d0. Read the comment docs.

cmd/issue_edit_test.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 27, 2018

Codecov Report

Merging #245 into master will increase coverage by 0.91%.
The diff coverage is 84.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   68.34%   69.26%   +0.91%     
==========================================
  Files          43       44       +1     
  Lines        2278     2414     +136     
==========================================
+ Hits         1557     1672     +115     
- Misses        562      573      +11     
- Partials      159      169      +10
Impacted Files Coverage Δ
internal/gitlab/gitlab.go 42.63% <50%> (+0.15%) ⬆️
cmd/issue_edit.go 86.71% <86.71%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd2388...4901315. Read the comment docs.

@claytonrcarter
Copy link
Collaborator Author

I just updated this use to use --assignee instead of --assign to be more consistent with mr create. Anything else I can do to help move this along?

@zaquestion
Copy link
Owner

@claytonrcarter Sorry I'm just looking at this again since last time. I think --assign make more sense here, because issues support multiple assignees while merge requests only support 1. When merge requests get updated to support multiple (I think still pending?) I figured we'd update merge requests to use --assign as well.

Do you think it makes sense to change back with that in mind? Read to merge this when I hear back.

zaquestion
zaquestion previously approved these changes Nov 28, 2018
@zaquestion
Copy link
Owner

@claytonrcarter forgot I added you as a collaborator. Approved the PR, feel free to merge at your discretion.

@claytonrcarter
Copy link
Collaborator Author

Makes sense! I reverted the last commit and will merge later today. Thanks!

@claytonrcarter claytonrcarter merged commit 09324ac into zaquestion:master Nov 29, 2018
@claytonrcarter claytonrcarter deleted the issue-edit branch November 29, 2018 00:45
@zaquestion zaquestion added this to the 0.15.0 milestone Nov 29, 2018
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.

3 participants