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.

Sign up for GitHub

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

Jump to bottom

cf service: Update headers to make them more consistent #2150

Merged
merged 3 commits into from
Mar 3, 2021
Merged

cf service: Update headers to make them more consistent #2150

merged 3 commits into from
Mar 3, 2021

Conversation

FelisiaM
Copy link
Member

@FelisiaM FelisiaM commented Mar 2, 2021

  • Updated headers
  • Indented data on each block

#176901932

@FelisiaM
cf service: Update headers to make them more consistent
77c2c4a 
- Updated headers
- Indented data on each block
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177164120

The labels on this github issue will be updated when the story is started.

blgm
blgm requested changes Mar 2, 2021
View reviewed changes
Copy link
Member

@blgm blgm left a comment

Choose a reason for hiding this comment

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

Looks good.

Don't really like adding the indent literal every time it's used. I'd prefer:

const indent = "   "
cmd.UI.DisplayKeyValueTable(indent, table, 3)
cmd.UI.DisplayText(indent + "This service instance is not currently being shared.")

What do you think?

@blgm
Copy link
Member

blgm commented Mar 2, 2021

Actually I prefer using a constant for padding too:

cmd.UI.DisplayKeyValueTable(indent, table, ui.DefaultTableSpacePadding)

@FelisiaM FelisiaM requested a review from blgm March 2, 2021 17:08
blgm
blgm requested changes Mar 2, 2021
View reviewed changes
command/v7/service_command.go Outdated
}
if serviceInstanceWithDetails.SharedStatus.FeatureFlagIsDisabled || serviceInstanceWithDetails.SharedStatus.OfferingDisablesSharing {
if serviceInstanceWithDetails.SharedStatus.FeatureFlagIsDisabled {
cmd.UI.DisplayText(` The "service_instance_sharing" feature flag is disabled for this Cloud Foundry platform.`)
Copy link
Member

Choose a reason for hiding this comment

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

Indent

command/v7/service_command.go Outdated
status := fmt.Sprintf("%s %s", serviceInstanceWithDetails.LastOperation.Type, serviceInstanceWithDetails.LastOperation.State)
table := [][]string{
{cmd.UI.TranslateText("status:"), status},
{cmd.UI.TranslateText("message:"), serviceInstanceWithDetails.LastOperation.Description},
{cmd.UI.TranslateText("started:"), serviceInstanceWithDetails.LastOperation.CreatedAt},
{cmd.UI.TranslateText("updated:"), serviceInstanceWithDetails.LastOperation.UpdatedAt},
}
cmd.UI.DisplayKeyValueTable("", table, 3)
cmd.UI.DisplayKeyValueTable(indent+"", table, ui.DefaultTableSpacePadding)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a mistake. If there's a type problem, could a cast work?

@FelisiaM FelisiaM requested a review from blgm March 3, 2021 09:25
@FelisiaM FelisiaM merged commit 3f42292 into master Mar 3, 2021
@FelisiaM FelisiaM deleted the v8_service_update_block_headers_176901932 branch March 3, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blgm blgm blgm approved these changes

Assignees
No one assigned
Labels
accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FelisiaM @cf-gitbot @blgm