issue1113

[SimonDold]

[issue1113] add check for multiple parameter occurrences.
The RawRegistry detects if a feature uses two arguments with the same name and throws an error.
Additionally, we remove the duplicate use of the verbosity argument in the ipdb feature.

[FlorianPommerening]

rename to argument_key_occurrences?

[maltehelmert]

If we're polishing terminology: in a function call like foo(x=3), the word "argument" should only really be used in the context of "3", while "x" is called the "parameter". Using the word "argument_key" instead of parameter feels a bit weird because of the two words "parameter"/"argument", "parameter" is arguably the more common and more widely understood one. So we're avoiding the use of the "normal" word parameter by inventing a new, more complex word "argument key".

[SimonDold]

The class Feature has the field std::vector<ArgumentInfo> arguments. The class ArgumentInfo has the field std::string key. I agree that the terminology would be better with:
Feature has the field std::vector<ParameterInfo> parameters and the class ParameterInfo has the field std::string name.

However, I am not sure if that would be a different issue to fix the terminology in this regard... but it would be fine for me to do it here.

[maltehelmert]

Generally I would say polishing things along the way is encouraged. I was just commenting because introducing a new variable "argument_key_occurrences" as suggested above seems jarring terminology to me, but I also don't want to make things too complicated. Whatever works for you.

[FlorianPommerening]

If you update this, I would say it should be updated throughout the code. Maybe to a grep for Argument and arg in the plugin directory.

[maltehelmert]

Sounds good, but for polishing issues like these, we've decided some time ago to only create issues if/when we actually want to follow through on them. The past has shown that if we create them and then let them sit in the tracker, nobody ever picks them up and they just sit in the tracker. So if you (or someone else reading this) wants to work on this, we should keep the issue; otherwise in our current workflow we wouldn't have it. (Note that this decision depends on the kind of issue -- this policy is one we follow primarily for "wishes" and polishing that one can take or leave.)

[FlorianPommerening]

I somewhat dislike using parameter only here and argument_key everywhere else, if nobody plans to make the change in the rest of the code. While it improves the code by using a better name, it also makes it inconsistent by only using this name in one of the many places where the concept is used. I'm all for the change, if someone is planning to do the follow-up work, though.

But since I'm not volunteering to do the work, I also wouldn't fight over this.

[SimonDold]

yes i would like to have a correct terminology in the rest of the code, too. And I am willing to work on that.
For the meantime we can either use the commit b5fc740
to have it more fitting to the rest of the code
or commit
7cd7006
to be more correct with the terminology.

I think option 1 would be better as changing the terminology would be one big separate issue then.

[FlorianPommerening]

Great, if you are willing to work on this, then I don't mind too much which version we merge now. If we have an issue for the remaining renaming and someone planning to eventually work on that, this is good enough to accept a temporary inconsistency.

[SimonDold]

https://issues.fast-downward.org/issue1114

[maltehelmert]

Before you merge, can you change "Parameter" to "parameter" in the message? (The existing messages can also be criticized for the same kind of grammatical mistake, but we can argue that we think of "Plugin" as a technical proper noun in this context. For "parameter" we can't.)