Feature toggle #460

msftrubengu · 2020-06-25T18:18:25Z

Change

Implementation for #364

Experimental features enable allow features to be distributed to early adopters for their feedback without the consequences of affecting already released functionalities.

Developers

The first in the workflow of a developer when implementing a new feature is to add a new Feature in ExperimentalFeature.h and follow the instruction in the comment. For commands or arguments, the disable protection is already in place. If the feature requires additional checks use ExperimentalFeatures::isEnabled(<your feature>).

When a feature is ready to be release, the enum value must be removed and all the reference to it. By always using the feature enum value we guarantee a cleaner removal.

To provide guidance, winget includes two test experimental features.

Users

To enable an experimental feature, go to the settings file and enable it under the experimentalFeatures property. Settings.md must have information about all the existing experimental feature (there are currently none).

To see all the available features, a user can execute the hidden winget features and see the status, as well as a link to the spec for the feature.

Validation

Implement new unittest and test locally the experimental test features

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS · 2020-06-26T22:37:11Z

src/AppInstallerCLICore/Argument.h

@@ -31,7 +32,7 @@ namespace AppInstaller::CLI
    };

    // Controls the visibility of the field.
-    enum class Visibility
+    enum class VisibilityArg


VisibilityArg [](start = 15, length = 13)

I would suggest moving it under Argument rather than just smooshing names together. #Closed

JohnMcPMS · 2020-06-26T22:41:24Z

src/AppInstallerCLICore/Argument.cpp

@@ -91,4 +94,14 @@ namespace AppInstaller::CLI
        args.push_back(ForType(Args::Type::RetroStyle));
        args.push_back(ForType(Args::Type::VerboseLogs));
    }
+
+    void Argument::OutputEnableMessage(Execution::Context& context) const


OutputEnableMessage [](start = 19, length = 19)

This should not be here, at least as a direct output function. If I'm guessing at its use, then it should generated a string to be part of the CommandException generated by parsing. #Closed

JohnMcPMS · 2020-06-26T22:48:03Z

src/AppInstallerCLICore/Command.cpp

@@ -162,8 +164,11 @@ namespace AppInstaller::CLI

            for (const auto& command : commands)
            {
-                size_t fillChars = (maxCommandNameLength - command->Name().length()) + 2;
-                infoOut << "  "_liv << Execution::HelpCommandEmphasis << command->Name() << Utility::LocIndString{ std::string(fillChars, ' ') } << command->ShortDescription() << std::endl;
+                if ((command->Visibility() == VisibilityCmd::Show) && User().isEnabled(command->Feature()))


if ((command->Visibility() == VisibilityCmd::Show) && User().isEnabled(command->Feature())) [](start = 16, length = 91)

Might be better to encode the feature check into Command::Visibility (and same for Argument::Visibility) #Closed

JohnMcPMS · 2020-06-26T22:51:10Z

src/AppInstallerCLICore/Command.cpp

@@ -199,16 +204,21 @@ namespace AppInstaller::CLI

            if (hasArguments)
            {
-                infoOut << Resource::String::AvailableArguments << std::endl;
-
+                bool atLeastOne = false;


bool atLeastOne = false; [](start = 16, length = 24)

Shouldn't be needed if the above code takes Visibility into account properly (as it does not right now). #Closed

JohnMcPMS · 2020-06-26T22:57:11Z

src/AppInstallerCLICore/Command.cpp

+
+        auto featureInfo = UserSettings::GetFeatureInfo(m_feature);
+        context.Reporter.Warn() << Resource::String::CommandDisabled << " " << m_name << std::endl;
+        context.Reporter.Warn() << Resource::String::ExperimentalFeatureName << " " << std::get<0>(featureInfo) << std::endl;


context.Reporter.Warn() [](start = 8, length = 23)

No need to get a new output object, just use:

Warn() <<
stuff << endl <<
more stuff << endl;

Also, it probably is not a great idea to try and inject multiple strings into the middle of localized text like this. Much better to have a single localized string and value after it. Something like (and get PMs to weigh in too probably):

This command is a work in progress, and may be changed dramatically or removed altogether in the future. To enable it, edit your settings ('winget settings') to include the experimental feature: <feature name string> ``` #Closed

JohnMcPMS · 2020-06-26T23:04:37Z

src/AppInstallerCLICore/Commands/FeaturesCommand.h

+    struct FeaturesCommand final : public Command
+    {
+        // This command is used as an example on how experimental features can be used.
+        // To enable this command set ExperimentalCmd = true in the settings file.


Copy pasta #Closed

JohnMcPMS · 2020-06-26T23:05:06Z

src/AppInstallerCLICore/Commands/FeaturesCommand.cpp

+
+    std::string FeaturesCommand::HelpLink() const
+    {
+        return "https://aka.ms/winget-seattings";


https://aka.ms/winget-seattings [](start = 16, length = 31)

Incorrect copy pasta #Closed

This was intentional. Are we saying we will have a special md file/microsoft docs page for this hidden command? I was thinking settings.md will have in the experimentalFeatures entry something like "to know which experimental features are available run winget features"

In reply to: 446445866 [](ancestors = 446445866)

I guess the link can point to #experimentalFeatures to be clear

In reply to: 447183011 [](ancestors = 447183011,446445866)

JohnMcPMS · 2020-06-26T23:07:29Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

+  </data>
+  <data name="FeaturesMessage" xml:space="preserve">
+    <value>The following experimental features are in progress.
+They can be configured through the settings file (winget settings).</value>


winget settings [](start = 50, length = 15)

I think putting the ' ' quotes around it makes it look more like a command to run. #Closed

JohnMcPMS · 2020-06-26T23:09:11Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

+    <value>If you want to try this feature, please go to winget settings and enable it. See</value>
+  </data>
+  <data name="FeaturesCommandLongDescription" xml:space="preserve">
+    <value>Shows the status of experimental features. Experimental features can be turn on via winget settings.</value>


winget settings [](start = 95, length = 15)

You will have to put a "lock" on this, see this elsewhere in the file:
{Locked="VirtualTerminal"} #Closed

JohnMcPMS · 2020-06-26T23:10:09Z

src/AppInstallerCLICore/Commands/FeaturesCommand.cpp

+        {
+            // Better work hard to get some out there!
+            context.Reporter.Info() << Resource::String::NoExperimentalFeaturesMessage << std::endl;
+        }


Feels like everything should be inside this if/else #Closed

JohnMcPMS · 2020-06-26T23:12:07Z

src/AppInstallerCLICore/Commands/ExperimentalCommand.cpp

+            else
+            {
+                // Here we warn the user that the argument exists and is not enabled.
+                Argument::ForType(Execution::Args::Type::ExperimentalArg).OutputEnableMessage(context);


This type of thing should be done either at parsing or by Command::Execute. #Closed

JohnMcPMS · 2020-06-26T23:14:29Z

src/AppInstallerCommonCore/JsonUtil.cpp

+    template<>
+    std::optional<bool> GetValue(const Json::Value& node)
+    {
+        std::optional<bool> value = std::nullopt;


= std::nullopt [](start = 33, length = 15)

This is the default constructor's behavior already. #Closed

JohnMcPMS · 2020-06-26T23:17:08Z

src/AppInstallerCommonCore/Public/winget/UserSettings.h

@@ -106,6 +157,8 @@ namespace AppInstaller::Settings
        }

        static std::filesystem::path SettingsFilePath();
+        static std::tuple<std::string, std::string, std::string> GetFeatureInfo(ExperimentalFeature feature);


std::tuple<std::string, std::string, std::string> [](start = 15, length = 49)

Don't use a tuple, take the time to create a struct with 3 names so that we can actually read the code. #Closed

JohnMcPMS · 2020-06-26T23:18:38Z

src/AppInstallerCommonCore/Public/winget/UserSettings.h

@@ -106,6 +157,8 @@ namespace AppInstaller::Settings
        }

        static std::filesystem::path SettingsFilePath();
+        static std::tuple<std::string, std::string, std::string> GetFeatureInfo(ExperimentalFeature feature);
+        static std::vector<std::tuple<std::string, std::string, std::string>> GetFeaturesInfo();


static std::vector<std::tuple<std::string, std::string, std::string>> GetFeaturesInfo(); [](start = 8, length = 88)

I don't really agree with tacking the features directly into the UserSettings. It is definitely a layer on top of them, and so it should be a separate type. #Closed

JohnMcPMS · 2020-06-26T23:19:10Z

src/AppInstallerCommonCore/Public/winget/UserSettings.h

+
+            // Not needed
+            // static std::optional<value_t> Validate(const json_t& value);
+        };


Special case sense is tingling...

And confirmed by looking at the cpp. These should be individual values, treated the same as any other. The features type should layer on top and handle the mapping from feature enum to setting enum. #Closed

JohnMcPMS

🕐

JohnMcPMS · 2020-06-30T19:57:07Z

src/AppInstallerCLITests/UserSettings.cpp

@@ -271,3 +271,44 @@ TEST_CASE("SettingAutoUpdateIntervalInMinutes", "[settings]")
        REQUIRE(userSettingTest.GetWarnings().size() == 1);
    }
 }
+
+// Test one experimental feature in usersettingstest context because there's no good way to test ExperimentalFeature


ExperimentalFeature [](start = 97, length = 19)

I think that you could have tests, but you would either need your Reload function back, or a test hook for features that allows you to inject your own settings object to be the one it uses. #WontFix

I kind of gave up after seeing all the amounts of changes i was doing plus me not wanting to mess with the constness of UserSettings::Instance

In reply to: 447942903 [](ancestors = 447942903)

JohnMcPMS · 2020-06-30T20:44:13Z

src/AppInstallerCommonCore/ExperimentalFeature.cpp

+        {
+            // Commented out on purpose these are just to provide example on how experimental features work.
+            // GetFeature(Feature::ExperimentalCmd),
+            // GetFeature(Feature::ExperimentalArg)


You can do this using the make_integer_sequence and folding, even if you want to exclude some you can use a Max that isn't at the end (these test ones are after). #Pending

ranm-msft · 2020-07-02T02:05:29Z

add , at the end (like in line 18) #Resolved

Refers to: doc/Settings.md:37 in 2c407ae. [](commit_id = 2c407ae, deletion_comment = False)

ranm-msft · 2020-07-02T02:05:44Z

doc/Settings.md

+       "experimentalCmd": true,
+       "experimentalArg": false,
+   }


} [](start = 3, length = 1)

add coma #Resolved

ranm-msft · 2020-07-02T02:10:44Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -383,6 +412,9 @@
  <data name="TagArgumentDescription" xml:space="preserve">
    <value>Filter results by tag</value>
  </data>
+  <data name="ThankYou" xml:space="preserve">
+    <value>Thank you from using winget</value>


from [](start = 21, length = 4)

for #Resolved

ranm-msft

JohnMcPMS

msftrubengu added 5 commits June 23, 2020 13:53

Add jsonutil bool

53e3679

Feature toggle setting

3687a82

Implement experimental features for demostration

41aec7e

Merge branch 'master' into featuretoggle

d2e49fa

fix bad merge

f466cc2

JohnMcPMS reviewed Jun 26, 2020

View reviewed changes

JohnMcPMS requested changes Jun 26, 2020

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Jun 26, 2020

Merge branch 'master' into featuretoggle

f3fcaaa

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Jun 29, 2020

PR comments and tests

dd17c26

msftrubengu marked this pull request as ready for review June 30, 2020 19:29

msftrubengu requested a review from a team as a code owner June 30, 2020 19:29

JohnMcPMS reviewed Jun 30, 2020

View reviewed changes

pr comments

2c407ae

ranm-msft reviewed Jul 2, 2020

View reviewed changes

ranm-msft approved these changes Jul 2, 2020

View reviewed changes

JohnMcPMS approved these changes Jul 2, 2020

View reviewed changes

msftrubengu added 2 commits July 2, 2020 10:47

typo

0a50b60

Merge branch 'master' into featuretoggle

2d389a7

msftrubengu merged commit 2750b58 into microsoft:master Jul 2, 2020

msftrubengu deleted the featuretoggle branch November 15, 2022 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature toggle #460

Feature toggle #460

msftrubengu commented Jun 25, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

msftrubengu Jun 29, 2020

msftrubengu Jun 29, 2020

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS left a comment

JohnMcPMS Jun 30, 2020 •

edited by msftrubengu

Loading

msftrubengu Jul 2, 2020

JohnMcPMS Jun 30, 2020 •

edited by msftrubengu

Loading

ranm-msft commented Jul 2, 2020 •

edited by msftrubengu

Loading

ranm-msft Jul 2, 2020 •

edited by msftrubengu

Loading

ranm-msft Jul 2, 2020 •

edited by msftrubengu

Loading

ranm-msft left a comment

JohnMcPMS left a comment

Feature toggle #460

Feature toggle #460

Conversation

msftrubengu commented Jun 25, 2020 • edited Loading

Change

Developers

Users

Validation

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

msftrubengu Jun 29, 2020

Choose a reason for hiding this comment

msftrubengu Jun 29, 2020

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

JohnMcPMS Jun 30, 2020 • edited by msftrubengu Loading

Choose a reason for hiding this comment

msftrubengu Jul 2, 2020

Choose a reason for hiding this comment

JohnMcPMS Jun 30, 2020 • edited by msftrubengu Loading

Choose a reason for hiding this comment

ranm-msft commented Jul 2, 2020 • edited by msftrubengu Loading

ranm-msft Jul 2, 2020 • edited by msftrubengu Loading

Choose a reason for hiding this comment

ranm-msft Jul 2, 2020 • edited by msftrubengu Loading

Choose a reason for hiding this comment

ranm-msft left a comment

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

msftrubengu commented Jun 25, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 26, 2020 •

edited

Loading

JohnMcPMS Jun 30, 2020 •

edited by msftrubengu

Loading

JohnMcPMS Jun 30, 2020 •

edited by msftrubengu

Loading

ranm-msft commented Jul 2, 2020 •

edited by msftrubengu

Loading

ranm-msft Jul 2, 2020 •

edited by msftrubengu

Loading

ranm-msft Jul 2, 2020 •

edited by msftrubengu

Loading