frej / fast-export

A mercurial to git converter using git-fast-import
http://repo.or.cz/w/fast-export.git
808 stars 255 forks source link

no difference between empty and no plugin option #290

Closed carandraug closed 1 year ago

carandraug commented 2 years ago

Assuming a plugin that takes a single option, at the moment it is not possible to distinguish between the empty string and no option. Example:

--plugin PluginName=foo    # opts is "foo"
--plugin PluginName=""      # opts is empty string
--plugin PluginName           # opts is empty string too

I think it would be nice to distinguish between the last two options. This would enable having a default value while also making it possible to use a value that is the empty string. Concrete case is PR #289 which adds a new plugin whose option is the prefix for a message. The default is hg: but this prevents the empty prefix case.

One possibility is to pass None. This has the problem that plugins have to handle None and empty strings. That can be implemented with something like:

index 93f35bf..c7221d1 100755
--- a/hg-fast-export.py
+++ b/hg-fast-export.py
@@ -715,8 +715,9 @@ if __name__=='__main__':
     sys.stderr.write('Using additional plugin path: ' + options.pluginpath + '\n')

   for plugin in plugins:
-    split = plugin.split('=')
-    name, opts = split[0], '='.join(split[1:])
+    split = plugin.split('=', maxsplit=1)
+    name = split[0]
+    opts = split[1] if len(split) > 1 else None
     i = pluginloader.get_plugin(name,options.pluginpath)
     sys.stderr.write('Loaded plugin ' + i['name'] + ' from path: ' + i['path'] +' with opts: ' + opts + '\n')
frej commented 2 years ago

My first thought was that this is an edge case, but then I looked at #289 where this functionality would be useful. Why not apply the above patch (you would have to update the existing plugins to not crash on None) as the first commit in #289 and then revise 502e146f36b245a5ad5db95b2930a650ef7d569b to default to a hg: prefix, but allow an empty prefix to be given on the command line?

carandraug commented 2 years ago

Sure, I can add the patch above to my other PR. However, here's an alternative view (against my own proposed solution) which I would like you to comment on first.

This issue is only relevant for plugins that have no named options. An alternative to the above is to require named options, i.e:

--plugin PluginName="OptionValue"  # positional option
--plugin PluginName='OptionName="Option Value"  # named option

which makes it clear when the option value is empty:

--plugin pluginName="OptionName="  # option value is clearly an empty string
--plugin pluginName  # option value is not defined so use default

This may even be better in the future since it makes it easier to add more options or change how options work later.

The problem with named options is that there's no single way to specify them and it can get tricky to parse certain strings as values. Suppose someone wants to use = in the string value, using #289 as example, someone wants hg= as prefix. I can't just split on =. Certainly we can code something up that handles but would be nice to have hg-fast-export.py define something that could be used by all plugins rather than have each plugin do its own parsing. Also makes it easier for the user who then only need to have a list of options for each plugin.

A possibility for something common is a JSON object since that's relatively simple and supported in Python core. So, using the example above:

--plugin pluginName="{'OptionName' : ''}"  # option value is clearly an empty string

I can also send a PR that implements the above if you agree that this is the right way to go.

frej commented 2 years ago

I think we're over-complicating things, as of now only we have only seen #289 as a plugin needing any advanced option parsing. I don't think it is worth the effort to try to implement something generic that will work for all future plugins, and I don't like JSON as command line syntax.

Why not, instead of trying to come up with the world's best and most flexible options parser :), just change the options syntax a little?

If you want to use the default tag, you just say: --plugin hg_hash_in_commit and the args inside the plugin will be "" which makes you default to "hg:" as the tag.

To use "foo" as a tag, you say --plugin hg_hash_in_commit=tag="foo" and the args inside the plugin will be `"tag=foo"

To give an empty tag, you say --plugin hg_hash_in_commit=tag="" and the args inside the plugin will be "tag="

So if you run args.split('=') you will in the three cases get [''], ['tag', 'foo'], and "['tag', '']" respectively.

frej commented 1 year ago

No activity in over 6 months, closing.