Make help be consistent. #1169

BillyONeal · 2023-08-26T10:47:11Z

This PR fixes the remaining issues noted in #1164

Command synopsis and example information are merged into CommandMetadata
This is also used to power autocomplete
- Note that this fixed a bug: autocomplete thought the command was vcpkg buildexternal but it is vcpkg build-external
This is also used to power help
The usage text for no arguments is moved into commands.cpp. This is now the one file in the codebase that has to include all the other commands' headers.
Pass around MessageT<> where possible rather than needing to make a thunk for everything.

Future work that would be nice I'm not signing up to do:

Make autocomplete more aggressive, for example by:

    enum class AutocompleteArguments
    {
        None,
        BuiltinPortNames,
        BuiltinPortNamesAfterPort,
        PortSpecs,
        InstalledPortSpecs,
        Integrate
    };

Make autocomplete work on 'frontend' options like --vcpkg-root
Make help text not suggest irrelevant 'fontend' options even though they are accepted

Before and after screenshots:

No arguments:

help install:

Autocomplete:

autoantwort · 2023-08-26T11:01:50Z

Always funny to learn about new commands even after years :D
The documentation for vcpkg cache is "list names of installed ports", but:

➜  vcpkg git:(cityhash-use-supports) ✗ vcpkg x-set-installed 'boost'                                          
Detecting compiler hash for triplet arm64-osx...
All requested packages are currently installed.
Total install time: 54.2 us
➜  vcpkg git:(cityhash-use-supports) ✗ vcpkg cache                  
No packages are cached.

BillyONeal · 2023-08-27T04:29:27Z

Always funny to learn about new commands even after years :D
The documentation for vcpkg cache is "list names of installed ports", but:

➜  vcpkg git:(cityhash-use-supports) ✗ vcpkg x-set-installed 'boost'                                          
Detecting compiler hash for triplet arm64-osx...
All requested packages are currently installed.
Total install time: 54.2 us
➜  vcpkg git:(cityhash-use-supports) ✗ vcpkg cache                  
No packages are cached.

I think this thing might just be broken and maybe should be deleted. I'll ask @vicroms if anyone is running this thing....

BillyONeal · 2023-08-29T23:14:40Z

Review comments on the usage:

@ras0219-msft wants to drop "Commands:"
@ras0219-msft wants to not need to repeat vcpkg in front of each command
The above two mean we need a one line 'breadcrumb' at the top "usage: vcpkg <command> [<args>...]"
Either they should all have periods or all not
@ras0219-msft wants a vcpkg help commands that lists all commands and remove artifacts commands from vcpkg help.
@AugP would like the headers to have the same capitalization. (For more help is lowercase and the others are capitals)
@ras0219-msft Would like a new vcpkg help commands that lists the full list like this, and to remove artifacts commands from the 'vcpkg with no arguments' list given that we don't expect to invest in artifacts soon.

Clean up needing to make thunks for all help text in command metadata machinery.

…ookup. Resolves this line item first discovered in microsoft#1164

…mandMetadata.

BillyONeal · 2023-08-30T21:19:51Z

The force push I just pushed should be no changes, just rebasing off the stuff that was identical to #1165

ras0219-msft

LGTM.

include/vcpkg/vcpkgcmdarguments.h

ras0219-msft · 2023-08-30T22:01:48Z

include/vcpkg/vcpkgcmdarguments.h

+        }
+
+        template<class Callback,
+                 std::enable_if_t<std::is_convertible_v<const Callback&, LocalizedString (*)()>, int> = 0>


For my own education: why does this use const Callback& instead of Callback by value?

I think they are equivalent in this case. I just default to const T& unless I have a specific reason to do otherwise when writing templates.

Note that even if the callback is Callback callback rather than const Callback& callback, the is_convertible_v still needs to have a & since we are converting the callback as an lvalue.

(Also it's smashed to a function pointer at constexpr time so there's no runtime difference; given that all the other ctors take references I'd like to leave this as-is because overload resolution that involves overloads that bind a reference vs. overloads that do not makes my brain hurt...)

ras0219-msft · 2023-08-30T22:12:26Z

src/vcpkg/commands.autocomplete.cpp

+                }
+            }
+
+            // If no public commands match, try intenral commands


Suggested change

// If no public commands match, try intenral commands

// If no public commands match, try internal commands

ras0219-msft · 2023-08-30T22:20:30Z

src/vcpkg/commands.env.cpp

+        {
+            "vcpkg env --triplet x64-windows",
+            msgCommandEnvExample2,
+            "vcpkg env \"ninja -C build\" --triplet x64-windows",


Suggested change

"vcpkg env \"ninja -C build\" --triplet x64-windows",

"vcpkg env \"ninja --version\" --triplet x64-windows",

A relative path argument to ninja is a bit fraught here since env doesn't retain the outer CWD.

ras0219-msft · 2023-08-30T22:21:48Z

src/vcpkg/commands.help.cpp

+        auto msg = msg::format(msgAvailableHelpTopics);
+        for (auto&& topic_name : all_topic_names)
+        {
+            msg.append_raw(fmt::format("\n  {}", topic_name));


Should this use LocalizedString::append_indent()?

Suggested change

msg.append_raw(fmt::format("\n {}", topic_name));

msg.append_raw("\n").append_indent().append_raw(topic_name);

Yes, I'll change it.

GitHub diff fail, I didn't meaningfully change this line from line 177 in the old code.

ras0219-msft · 2023-08-30T22:37:44Z

src/vcpkg/vcpkgcmdarguments.cpp

@@ -86,7 +86,7 @@ namespace

            if (switch_.helpmsg)
            {
-                if (cmd_parser.parse_switch(name, tag, parse_result, switch_.helpmsg()) && parse_result)
+                if (cmd_parser.parse_switch(name, tag, parse_result, switch_.helpmsg.format()) && parse_result)


(Sorry for not doing the investigation myself)

How does this avoid crashing for switches that have default constructed help (e.g. remove's OPTION_PURGE)?

The if on line 87 will be false because operator bool on that thing returns false if there's nothing inside

ras0219-msft · 2023-08-30T22:38:41Z

src/vcpkg/vcpkgcmdarguments.cpp

+        }
+    }
+
+    void MetadataMessage::append_to(LocalizedString& target) const


Suggested change

void MetadataMessage::append_to(LocalizedString& target) const

void MetadataMessage::format_to(LocalizedString& target) const

(suggestion)

I think format_to carries the connotation that the target's contents will be destroyed. How about format_append_to?

Changed to to_string since we seem to use that convention elsewhere.

…ndent in all the right places for help. Also remove the "name" which need be localized from the help versioning example now that this is no longer required to avoid needing to localize it.

@ras0219-msft

* drop "Commands:" from @ras0219-msft * don't repeat `vcpkg ` in front of each command from @ras0219-msft * Add 'breadcrumb' for the above * change all synopsis to not end in periods from @BillyONeal * split rarely used and artifacts into separate `vcpkg help commands` from @ras0219-msft * always use Pascal Case headers from @AugP * make tense for all synopsis and argument help text consistent from @ras0219-msft * burninate 'instead of' and 'writes out'

ras0219-msft · 2023-09-05T23:28:44Z

include/vcpkg/vcpkgcmdarguments.h

+        template<int N>
+        /*implicit*/ constexpr LearnWebsiteLinkLiteral(const char (&literal)[N]) noexcept : literal(literal)
+        {
+            assert(!constexpr_contains(literal, "en-us") &&


Is there some sort of divide-by-zero or array-of-zero thing that can be done here to make this assert compile time?

It is compile time; assert is constexpr when the assertion passes and not constexpr when the assertion fails.

ras0219-msft · 2023-09-05T23:30:06Z

src/vcpkg-test/cmd-parser.cpp

-                                  going and going
+                         shorty
+  short-arg              some really long help text that does not fit on the same line because we
+                         have a 100 character line limit and oh god it keeps going and going


Suggested change

have a 100 character line limit and oh god it keeps going and going

have a 100 character line limit and oh no it keeps going and going

ras0219-msft · 2023-09-05T23:33:28Z

src/vcpkg/commands.cpp

+    void print_full_command_list()
+    {
+        HelpTableFormatter table;
+        format_command_usage_entry(table, CommandAcquireMetadata);


This seems easy to miss when adding new commands.

Do we have a list of all commands somewhere else we could loop through, skipping based on undocumented, etc?

ras0219-msft · 2023-09-05T23:35:35Z

src/vcpkg/commands.cpp

        format_command_usage_entry(table, CommandFetchMetadata);
-        format_command_usage_entry(table, CommandVersionMetadata);
-        format_command_usage_entry(table, CommandVsInstancesMetadata);
+        format_command_usage_entry(table, CommandIntegrateMetadata);
        table.blank();

        table.header(msg::format(msgForMoreHelp));
        table.format("vcpkg help topics", msg::format(msgHelpTopicsCommand));


Consider dropping vcpkg from these entries.

# Conflicts: # src/vcpkg/commands.cache.cpp # src/vcpkg/commands.cpp # src/vcpkg/commands.help.cpp

…g help blah'.

BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Aug 26, 2023

BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label Aug 30, 2023

BillyONeal added 5 commits August 30, 2023 14:18

Add name, synopsis, and example information to CommandMetadata.

8516098

Clean up needing to make thunks for all help text in command metadata machinery.

Power autocomplete and help with the same tables that power command l…

6c2594c

…ookup. Resolves this line item first discovered in microsoft#1164

Improve help by printing synopsis and grouped examples powered by Com…

44da838

…mandMetadata.

Fix test failures :)

05b05fe

Update the main usage to make sure every relevant command is included.

b64f222

BillyONeal force-pushed the merge-help-tables branch from d27369d to b64f222 Compare August 30, 2023 21:19

ras0219-msft approved these changes Aug 30, 2023

View reviewed changes

BillyONeal added 5 commits August 30, 2023 21:44

Correct the description of what 'cache' does as noticed by @autoantwort

f25c0e8

Small code review fixes from @ras0219-msft , including using append_i…

b0b74e2

…ndent in all the right places for help. Also remove the "name" which need be localized from the help versioning example now that this is no longer required to avoid needing to localize it.

Add website link to all help commands.

c97ba85

Fix unit test failure

1630665

BillyONeal requested a review from ras0219-msft August 31, 2023 07:54

ras0219-msft approved these changes Sep 5, 2023

View reviewed changes

BillyONeal added 4 commits September 6, 2023 17:01

Merge remote-tracking branch 'origin/main' into merge-help-tables

39a30cc

# Conflicts: # src/vcpkg/commands.cache.cpp # src/vcpkg/commands.cpp # src/vcpkg/commands.help.cpp

Deduplicate the all commands help topic and remove 'vcpkg' from 'vcpk…

51c3c30

…g help blah'.

No gods, only man

8e0e7ee

Also drop vcpkg from the localized messages.

dfb7a52

BillyONeal merged commit e6c0307 into microsoft:main Sep 7, 2023
5 checks passed

BillyONeal deleted the merge-help-tables branch September 7, 2023 06:55

BillyONeal mentioned this pull request Sep 7, 2023

Add help information for fetch command #1182

Closed

jimwang118 mentioned this pull request Sep 7, 2023

[vcpkg-tool] add fetch command to manual microsoft/vcpkg#31058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make help be consistent. #1169

Make help be consistent. #1169

BillyONeal commented Aug 26, 2023 •

edited

Loading

autoantwort commented Aug 26, 2023

BillyONeal commented Aug 27, 2023

BillyONeal commented Aug 29, 2023 •

edited

Loading

BillyONeal commented Aug 30, 2023

ras0219-msft left a comment

ras0219-msft Aug 30, 2023

BillyONeal Aug 31, 2023 •

edited

Loading

BillyONeal Aug 31, 2023

ras0219-msft Aug 30, 2023

ras0219-msft Aug 30, 2023

ras0219-msft Aug 30, 2023

ras0219-msft Aug 30, 2023

BillyONeal Aug 31, 2023

ras0219-msft Aug 30, 2023

BillyONeal Aug 31, 2023 •

edited

Loading

ras0219-msft Aug 30, 2023

BillyONeal Aug 31, 2023

BillyONeal Aug 31, 2023

ras0219-msft Sep 5, 2023

BillyONeal Sep 7, 2023

ras0219-msft Sep 5, 2023

ras0219-msft Sep 5, 2023

ras0219-msft Sep 5, 2023

	// If no public commands match, try intenral commands
	// If no public commands match, try internal commands

	"vcpkg env \"ninja -C build\" --triplet x64-windows",
	"vcpkg env \"ninja --version\" --triplet x64-windows",

	msg.append_raw(fmt::format("\n {}", topic_name));
	msg.append_raw("\n").append_indent().append_raw(topic_name);

	void MetadataMessage::append_to(LocalizedString& target) const
	void MetadataMessage::format_to(LocalizedString& target) const

	have a 100 character line limit and oh god it keeps going and going
	have a 100 character line limit and oh no it keeps going and going

Make help be consistent. #1169

Make help be consistent. #1169

Conversation

BillyONeal commented Aug 26, 2023 • edited Loading

autoantwort commented Aug 26, 2023

BillyONeal commented Aug 27, 2023

BillyONeal commented Aug 29, 2023 • edited Loading

BillyONeal commented Aug 30, 2023

ras0219-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal Aug 31, 2023 • edited Loading

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

BillyONeal Aug 31, 2023 • edited Loading

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

BillyONeal commented Aug 26, 2023 •

edited

Loading

BillyONeal commented Aug 29, 2023 •

edited

Loading

BillyONeal Aug 31, 2023 •

edited

Loading

BillyONeal Aug 31, 2023 •

edited

Loading