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

Add relevant metadata to issue view in CLI #745

Merged
merged 13 commits into from
Apr 9, 2020

Conversation

doi-t
Copy link
Contributor

@doi-t doi-t commented Apr 3, 2020

This is a part of #663 (issue view). I will open another PR for pr view to avoid gigantic PR.

(This PR is a nested PR of #667 so that #667 must be merged first.)
Thank you for merging the PR! πŸŽ‰ πŸ˜„

@ampinsk
Could you please review the design especially for the case of empty body and lots of metadata?

This is a practical example (gh issue view 663 --repo cli/cli). Please note that there is no Milestone because this issue is not assigned to any milestone.

Screen Shot 2020-04-03 at 22 44 17

This is an example of an opening issue without metadata settings (gh issue view 15 --repo doi-t/cli-test). Participants is set with the author's login account by default and, as far as I know, we cannot avoid it so that we will see it all the time.

Screen Shot 2020-04-03 at 22 39 48

This is an example of an opening issue with lots of labels. Labels has , … at the end of the line in this case because there are more than 3 labels. Since this issue does not have body content, there is only a new line between metadata and the issue's URL.

Screen Shot 2020-04-03 at 22 49 32

Regarding to multiple items, while Milestone can have only one milestone, Assignees, Labels, Projects and Participants could have multiple items. I set 3 as the threshold so far. If there are 4 or more items, each metadata will have , … at the end as shown below (This is just a generated example. Field names will be bolded in the actual output.).

Screen Shot 2020-04-03 at 23 05 53

@mislav mislav added this to In progress 🚧 in The GitHub CLI Apr 3, 2020
@ampinsk
Copy link
Contributor

ampinsk commented Apr 3, 2020

Hi @doi-t thank you for this awesome work! I'll get to looking it over shortly.

Quick thought/question though looking at this: is Participants helpful to have here at all? πŸ€”

cc @mislav @vilmibm @billygriffin if you have thoughts!

@doi-t
Copy link
Contributor Author

doi-t commented Apr 4, 2020

is Participants helpful to have here at all? πŸ€”

Yeah, I have the same feeling. Personally, I have never paid attention to Participants. Maybe, it matters when many people are involved in an issue or a PR. But I guess listing lots of account names is not what gh can/want to handle in CLI. πŸ€”

This is how issue view looks like if we omit Participants:

Screen Shot 2020-04-04 at 13 04 39

@doi-t doi-t marked this pull request as ready for review April 4, 2020 04:26
@billygriffin
Copy link
Contributor

Thanks @doi-t! I'm also in favor of omitting participants - it doesn't seem to add very much in the way of useful context unless we see evidence to the contrary. I'm also not sure how I feel about the truncation after three of each item. This is more of a provocative question than an actual suggestion, but what if we didn't truncate at all?

Separately, I'm curious from @mislav and @vilmibm if there are implications to this PR for scripting and machine readability?

@doi-t
Copy link
Contributor Author

doi-t commented Apr 7, 2020

@billygriffin Thank you for the review!

I'm also not sure how I feel about the truncation after three of each item. This is more of a provocative question than an actual suggestion, but what if we didn't truncate at all?

Actually, it is not what @ampinsk specifically guided at #663 (comment). I just applied the existing label list implementation which does the truncation for gh issue list to other metadata. I should have mentioned it. πŸ™‡I agree with no truncation! I usually want to see all of them. Projects with column name (and Reviewers with their state) could tend to be a long line, though. πŸ€”

@mislav
Copy link
Contributor

mislav commented Apr 8, 2020

I'm πŸ‘ removing participants; it doesn't seem very useful in this context.

Separately, I'm curious from @mislav and @vilmibm if there are implications to this PR for scripting and machine readability?

In my view, pr/issue view were not perfectly machine-readable to start with, and this PR does not have any negative implications that I can tell: it only adds a few more fields to the output.

I think that a machine-readable view command should ideally accept some kind of --format string or template so that the user can specifically request the information that they want to consume with their script. Or, if the command always outputs lots of different fields, those fields should be strictly delinated by some well-known format (e.g. JSON, YAML, TSV, etc.) and their values should never get adjusted processed for display (like we did with e.g. truncating labels with … when there were more than 3 labels).

@doi-t
Copy link
Contributor Author

doi-t commented Apr 8, 2020

@mislav
Through the implementation, I found that the truncation is done by the query. To address the design guide #663 (comment), I updated the query to get all metadata (first: 100) while I keep the fragments with first: 3 which truncates labels for gh issue list.
I wonder how you see the metadata output of the issue view without the truncation. I feel it is desirable because I see gh issue list as summary information and gh issue view as full information.

Here is the example of no truncation.
Screen Shot 2020-04-08 at 20 32 41

This PR is ready for code review too!

Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

I think we all agreed to remove Participants for now but other than that looking good! Thank you!!

Once participants is out I can merge.

The GitHub CLI automation moved this from In progress 🚧 to Needs to be merged πŸŽ‰ Apr 9, 2020
@vilmibm vilmibm moved this from Needs to be merged πŸŽ‰ to In progress 🚧 in The GitHub CLI Apr 9, 2020
@doi-t
Copy link
Contributor Author

doi-t commented Apr 9, 2020

@vilmibm
I've removed all Participants related codes 054ec3c ✨

@vilmibm
Copy link
Contributor

vilmibm commented Apr 9, 2020

thank you~

@vilmibm vilmibm merged commit c670049 into cli:master Apr 9, 2020
The GitHub CLI automation moved this from In progress 🚧 to Pending Release πŸ₯š Apr 9, 2020
@billygriffin
Copy link
Contributor

Hey @doi-t, just wanted to say that we've really appreciated all of your contributions to this project and how thoughtful and open to feedback you've been. Thanks so much! πŸ’–

@doi-t doi-t deleted the add-metadata-to-view branch April 10, 2020 01:50
@doi-t
Copy link
Contributor Author

doi-t commented Apr 10, 2020

@billygriffin
Thank you! It was an incredible experience working with the team. πŸ˜„ I'm still willingly open to any other contributions to this project if it's fine! πŸ™‹β€β™‚οΈ

@billygriffin
Copy link
Contributor

It's absolutely fine and wonderful to hear that you'd like to continue contributing. ❀️ Feel free to ask about upcoming things you're interested in, and we'll happily @-mention you if there are things where we're looking for community contributions, especially in areas you've already contributed. Thanks again!

@github-actions github-actions bot moved this from Pending Release πŸ₯š to Done πŸ’€ in The GitHub CLI Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done πŸ’€
Development

Successfully merging this pull request may close these issues.

None yet

5 participants