fw4spl-org / fw4spl

Main repository for fw4spl
30 stars 15 forks source link

Removed need of "uid" attribute in <parameter replace=... /> #4

Closed fbridault closed 7 years ago

fbridault commented 7 years ago

We can now use "by" each time, like in former appXml. The historical "GENERIC_UID" substitution is performed in AppConfig2 when parsing the configuration. I build a dictionnary of potential targets for a future replacement, and then I simply lookup when I parse a "by" attribute of a "parameter" tag. This is not very elegant, but I think removing this attribute is more important. We can rework the implementation later but in the current state, the choice between "uid" and "by" is really misleading and must be removed asap. Note that for backwards compatibility, it is still allowed to specify "uid", so people can migrate when they want. The code that translates the appXml2 configuration of the launcher has been merged from SConfigController and SConfigLauncher into ConfigLauncher. A unit test has been added to verify this.

emiliehar commented 7 years ago

It's a good idea to simplify the configuration.

I think we may have a problem in few configurations when we declare a service uid or a channel in the config launcher of the root config, but we really use it in the sub-configurations. It is possibly be a bad use of the "uid". For example, when a channel must be shared between two configurations but is not used in the root configuration.

fbridault commented 7 years ago

Indeed, nice catch. I didn't think about that, but you are right, we use that sometimes. Hopelessly with this approach, I think there is no clever way to determine that a generic identifier should be generated. My proposal would be thus, in these cases, to specify manually that a substitution should be done, let's say with a variable called ${UID}. ^^ Yeah it's kind of a jump in the past, but somehow, that's really what the programmer wants to say at this point :"I want to generate a unique ID that will be passed to my subconfigs". It might be even clearer this way, we can easily identify that these tags do not refer to objects inside the current config. Alternatively we could stick with "uid" attribute but I think it is clearer to use something like ${UID}.

fbridault commented 7 years ago

I forgot to mention that I can provide a short python script to convert the files we want to switch, and that would be easy to detect the tags that still need a "manual" uid generation.

fbridault commented 7 years ago

@emiliehar I updated the doxygen of the services yesterday evening, please tell me if that looks better for you.

fbridault commented 7 years ago

Despite the two approvals, I didn't merged this PR because of the issue mentionned by @emiliehar. I think we should agree on a strategy first, either keep the use of uid attribute, or rely on ${GENERIC_UID} or whatever {UID}.