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

add plugin headers to config #1411

Closed
wants to merge 3 commits into from

Conversation

oxarbitrage
Copy link
Member

#1407

Function create_new_config_file receives all the available command line options in order but it will not specify from what plugin they came from.

Here i add a vector of pairs plugin_name, first_plugin_option in order to place a header. I know it is a bit dirty but i think it can work.

New plugins will need to be added here but if they are not added the header will not be added and nothing more, config will be generated anyways.

Here is how it looks:

https://pastebin.com/raw/whb5UDch

@oxarbitrage oxarbitrage added 3b Feature Classification indicating the addition of novel functionality to the design 6 UX Impact flag identifying the User Interface (UX) 6 Plugin Impact flag identifying at least one plugin labels Oct 31, 2018
@HarukaMa
Copy link
Contributor

It seems witness is not listed...

@oxarbitrage
Copy link
Member Author

oxarbitrage commented Oct 31, 2018

It seems witness is not listed...

you are right, and also debug_witness.

please check how it is looking now with those 2 additions: https://pastebin.com/raw/8xcGAzSG

@pmconrad
Copy link
Contributor

pmconrad commented Nov 1, 2018

  • Section headers should be better visible, perhaps make them multi-line with a frame around or sth like that.
  • Not sure if the sorting order of options and/or sections is well-defined. If it isn't, your method of detecting the beginning of a new section won't work.
  • Listing all plugins explicitly is a bad idea. You should use those registered in the current application.

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

See other comment

@oxarbitrage
Copy link
Member Author

I was trying to get something better for points 2 and 3 of your review @pmconrad but i was not able to get anything that will fully convince me and probably you either. Here is what i think:

With all this i think that will make some sense it to move the map construction i added into programs/witness_node/main.cpp before calling load_configuration_options and pass it as argument to create_new_config_file.

load_configuration_options is only used there and tests/app/main.cpp test cases.

Any idea is appreciated as i am a bit stuck with this.

@pmconrad
Copy link
Contributor

pmconrad commented Nov 8, 2018

A simple alternative might be to move load_configuration_options (and whatever else it needs) into the application class. It doesn't have to be a standalone function, especially since it's part of graphene::app.

@nathanielhourt
Copy link
Contributor

nathanielhourt commented Nov 15, 2018

An alternative approach would be to require plugins to put their configuration options in a section named after the plugin:
https://www.boost.org/doc/libs/1_55_0/doc/html/program_options/overview.html#idp163379208

What I've always wanted to do is have the plugins set their options as they do now, but then bundle up each plugin's options into a section named after the plugin before inserting them into the global options_description in register_plugin():

         template<typename PluginType>
         std::shared_ptr<PluginType> register_plugin()
         {
            auto plug = std::make_shared<PluginType>();
            plug->plugin_set_app(this);

            boost::program_options::options_description plugin_cli_options(plug->plugin_name() + " plugin. " + plug->plugin_description() + "\nOptions"), plugin_cfg_options;
            plug->plugin_set_program_options(plugin_cli_options, plugin_cfg_options);
            if( !plugin_cli_options.options().empty() )
               _cli_options.add(plugin_cli_options);
            if( !plugin_cfg_options.options().empty() )
               _cfg_options.add(plugin_cfg_options);
// Instead of _xyz_options.add(options) above,
// something like _xyz_options.add_section("section_name", options)

            add_available_plugin( plug );
            return plug;
         }

Unfortunately, I can't find any way to actually do this. I've even looked for ways to transform the options to add the section to the name, but there is no clear way to copy a boost::program_options::option_description object with a modified name, due to the complexity of their names (long names vs short names, etc).

So instead, an alternative would be to simply require plugins to politely bundle their options into a group internally, and then verify that they did it correctly at the register_plugin() level.

Thoughts?

@oxarbitrage
Copy link
Member Author

Seems to me the section thing is the right way of doing it. I was not aware of the possibility.

I am not against a way that will require to add some little code to every plugin we have including template_plugin.

@pmconrad
Copy link
Contributor

That sounds like a good solution, if it works.

Perhaps also check https://www.boost.org/doc/libs/1_55_0/doc/html/program_options/howto.html#idp163421144

@abitmore
Copy link
Member

abitmore commented Mar 9, 2019

See #1641?

@oxarbitrage
Copy link
Member Author

replaced by #1641

@abitmore abitmore removed this from the Possible 201904 Feature Release milestone Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3b Feature Classification indicating the addition of novel functionality to the design 6 Plugin Impact flag identifying at least one plugin 6 UX Impact flag identifying the User Interface (UX)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants