fix: correct example in documentation #935

btrd · 2023-04-13T12:39:00Z

Add a changelog entry in the section "To Be Released" of CHANGELOG.md

⬆️ Do I need to update the changelog for a change this small?

Also I'm wondering if this command shouldn't use the --addon params like other command?

EtienneM

arrow_up Do I need to update the changelog for a change this small?

i would say it's OK not to have a changelog for this

EtienneM · 2023-04-13T12:48:52Z

cmd/addons.go

@@ -133,7 +133,7 @@ var (
 		ArgsUsage: "addon-id",
 		Description: CommandDescription{
 			Description: "Display information about an add-on attached to your app",
-			Examples:    []string{"scalingo --app my-app addons-info mongodb"},
+			Examples:    []string{"scalingo --app my-app addons-info addon_uuid"},


I would actually prefer if we could make the previous example work. We developed a fix for this use case for the logs command, we could do the same for the addons-info

In addition, we always give example names, like my-app instead of app-name.

This is already specified in the ArgsUsage that it is the addon-id that is expected.

If we want to change what we put for the addon id in the examples, we have to change it as well for every example in addon-ids.

We developed a fix for this use case for the logs command, we could do the same for the addons-info

logs is taking a --addon params, it's not the case here (it's what I'm proposing also).

I tried a to duplicate the logs behavior for the addons-info but didn't succesed, and I don't have the time currently to do it.

I would be in favor of accepting this PR, and wrote an issue to improve this command, in order to accept a params or a flag and a addon name or uuid.

OK I think you are right. I opened #937.

@yohann-bacha currently both addon_uuid and addon-uuid are in use, there is unfortunately no convention. Hence it looks good to me.

@yohann-bacha waiting for your review in order to merge this PR :)

@EtienneM My point was not about using a _ or a -, this is more a problem of consistency.

Currently for every example in addons.go, we are giving real names for addons. As mongodb, and not addon-id, neither addon_uuid.

If we use now addon_uuid this is fine, but we shall do this everywhere at least in the addons.go file.

The following point is not blocking, but I think examples should be real world commands that you can copy paste and use them. What we need as args must be in the ArgsUsage (they are already there), the examples should not be another ArgsUsage.

As mongodb, and not addon-id, neither addon_uuid.

I think you miss-understood something, mongodb is not an id or uuid it's a addon name. Here the user need to use a value that looks like this 23w2e3rjwefsdjl;fgnlsdnfgnsdgn23 (yes I put my head on the keyboard), so I don't think we should give a "real" example.

Could you Add a suggestion to this PR if I didn't understood what's you're proposing?

You are absolutely right, then in that case, in examples where an addon-id is required, there should always be addon_uuid then. Line 107, 77...

And for the addon-add command, the argusage is wrong : it is not the addon-id we are waiting for, but the addon name

You are absolutely right, then in that case, in examples where an addon-id is required, there should always be addon_uuid then. Line 107, 77...
And for the addon-add command, the argusage is wrong : it is not the addon-id we are waiting for, but the addon name

Indeed it seems there are other errors in this file :/

@yohann-bacha Thanks for the update!

@EtienneM can you review the new commit?

yohann-bacha · 2023-04-13T13:32:21Z

cmd/addons.go

@@ -133,7 +133,7 @@ var (
 		ArgsUsage: "addon-id",
 		Description: CommandDescription{
 			Description: "Display information about an add-on attached to your app",
-			Examples:    []string{"scalingo --app my-app addons-info mongodb"},
+			Examples:    []string{"scalingo --app my-app addons-info addon_uuid"},


In addition, we always give example names, like my-app instead of app-name.

This is already specified in the ArgsUsage that it is the addon-id that is expected.

If we want to change what we put for the addon id in the examples, we have to change it as well for every example in addon-ids.

EtienneM · 2023-04-18T08:16:42Z

cmd/addons.go

@@ -133,7 +133,7 @@ var (
 		ArgsUsage: "addon-id",
 		Description: CommandDescription{
 			Description: "Display information about an add-on attached to your app",
-			Examples:    []string{"scalingo --app my-app addons-info mongodb"},
+			Examples:    []string{"scalingo --app my-app addons-info addon_uuid"},


OK I think you are right. I opened #937.

@yohann-bacha currently both addon_uuid and addon-uuid are in use, there is unfortunately no convention. Hence it looks good to me.

He made the changes himself

fix: correct example in documentation

bf0f1c5

btrd requested a review from yohann-bacha April 13, 2023 12:39

EtienneM reviewed Apr 13, 2023

View reviewed changes

yohann-bacha previously requested changes Apr 13, 2023

View reviewed changes

EtienneM mentioned this pull request Apr 18, 2023

[addons-info] Should accept addon name as --addon parameter #937

Closed

EtienneM approved these changes Apr 18, 2023

View reviewed changes

EtienneM requested a review from yohann-bacha April 18, 2023 08:18

EtienneM enabled auto-merge April 18, 2023 08:18

fix: correct denominations for arguments and examples

96013f8

yohann-bacha requested a review from EtienneM April 19, 2023 09:24

EtienneM approved these changes Apr 20, 2023

View reviewed changes

EtienneM merged commit b36ff69 into master Apr 20, 2023

EtienneM deleted the fix_doc branch April 20, 2023 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correct example in documentation #935

fix: correct example in documentation #935

btrd commented Apr 13, 2023 •

edited

Loading

EtienneM left a comment

EtienneM Apr 13, 2023

yohann-bacha Apr 13, 2023

btrd Apr 17, 2023

EtienneM Apr 18, 2023

btrd Apr 18, 2023

yohann-bacha Apr 19, 2023

btrd Apr 19, 2023

yohann-bacha Apr 19, 2023

btrd Apr 19, 2023

btrd Apr 19, 2023 •

edited

Loading

yohann-bacha Apr 13, 2023

EtienneM Apr 18, 2023

fix: correct example in documentation #935

fix: correct example in documentation #935

Conversation

btrd commented Apr 13, 2023 • edited Loading

EtienneM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btrd Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btrd commented Apr 13, 2023 •

edited

Loading

btrd Apr 19, 2023 •

edited

Loading