Should the path and version of a ModuleConditionalResource be mandatory?

Hi there,

while testing, refactoring the ModuleFileParser which turns a config.xml into a Module I found that

<conditionalResource>
</conditionalResource>

is allowed which leads to the creation of an instance of ModuleConditionalResource with no path and no version.

I think the path and version should be mandatory, correct? (A search on github under the openmrs org for conditionalResource shows conditionalResource always comes with a path and a version).

If you agree I propose to deprecate the default constructor of ModuleConditionalResource in favor of ModuleConditionalResource(String path, String version) with appropriate checks and tests so we make it easy for developers to do the right thing and not create instances with an invalid state. I would then also adjust the ModuleFileParser so we do not allow such an invalid conditionalResource.

Sounds vaguely correct to me, at first glance.

I believe conditional resource loading is version sensitive already.

As for the path, I’d think conditional resources are assumed to be classpath resources, so the path is already included in the resource name anyways.

I think you mean something else, I am talking about

  1. the java class ModuleConditionalResource. Specifically that we allow creation of a conditional resource object instance that is in my view invalid with new ModuleConditionalResource(). This instance has no path and no version which I think should be mandatory, what do you think?

  2. that we allow such an empty conditional resource to be defined in the modules config.xml which should in my view not be allowed for the same reason as 1.

I suggest to fix this, so we make it clear and easy for devs to use class ModuleConditionalResource and also to write correct module config.xml’s