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

fix: inconsistent format between issue list and status #532

Merged
merged 2 commits into from
Mar 18, 2020

Conversation

mingrammer
Copy link
Contributor

Currently, issue list and issue status have different printing formats.

This PR adds the age section for issue list and uses the tablewriter for issue status to keep the format consistency between them.

Current version

Screen Shot 2020-02-23 at 6 41 08 PM

Fixed version

Screen Shot 2020-02-23 at 6 41 00 PM

@mislav mislav requested a review from ampinsk February 24, 2020 12:13
Copy link
Contributor

@ampinsk ampinsk left a comment

Choose a reason for hiding this comment

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

This looks good to me from a design perspective! @mislav or @vilmibm should take a look code wise 😄

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Nice work! Just few little suggestions, plus a request: since this tweaks the output of issue status, could you apply the same improvements to pr status?

Thanks! This looks great ✨

command/issue.go Show resolved Hide resolved
command/issue.go Outdated Show resolved Hide resolved
@mingrammer
Copy link
Contributor Author

PR status has optional checks and review status. Should I make these as table fields? :)

@mislav
Copy link
Contributor

mislav commented Mar 9, 2020

PR status has optional checks and review status.

Hmm that's a good point. Right now it might be tricky to force those into table fields. Please disregard my last comment and hold off porting pr status to table view until we decide what to do here 🤔

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

This looks good if we just consider issue status output, but I would hold off merging until we figure out whether it's a problem that this makes the output different than pr status, and discuss how could we apply the same formatter there—PR status has more fields, so it might be more complicated to force it to a table layout.

@mingrammer
Copy link
Contributor Author

Yap. I'm glad that this pr is approved :) I'll be waiting for final decisions.

@billygriffin billygriffin requested a review from ampinsk March 11, 2020 18:51
@ampinsk
Copy link
Contributor

ampinsk commented Mar 11, 2020

I think since the info and layout of pr status is more complex we shouldn’t force the table layout. So I think this is good to go!

@mislav mislav merged commit cb06812 into cli:master Mar 18, 2020
@mingrammer mingrammer deleted the consistent-printing branch March 18, 2020 11:00
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