Fail if an enumerated argument does not lie within a given set of values

[jason-fox]

Description

• Simplified ANT scripts by removing unnecessary <condition> statements.
• Add new failure message DOTA015F
• Checked for parameter values using <matches> condition

Motivation and Context

Adds a check for enumerated parameters. Fails if parameter is not in the required range.Fixes #1935.

How Has This Been Tested?

Manual testing. Ran xhtml transform with and without valid generate.copy.outer parameter

Type of Changes

• New feature : Parameter checking - currently added to 3 enumerated parameters

Checklist

I have not found any coding convention for ANT scripts.
No unit test has been added.

[stefan-jung]

LGTM, is easier to read

[jelovirt]

I think I prefer declarations with checks more, because they are explicit in that "this property may have been set earlier". Plain

<property name="dita.xhtml.reloadstylesheet" value="false"/>

is shorter and thus easier to read, but this leads to reading it as "set dita.xhtml.reloadstylesheet to false", when in reality it's "set dita.xhtml.reloadstylesheet to false unless it's already been set". The reader is assumed to know that this is how Ant work, but it's easy to forget.

<condition property="dita.xhtml.reloadstylesheet" value="false">
  <not><isset property="dita.xhtml.reloadstylesheet"/></not>
</condition>

Is explicit and reminds you that the value may not be set in all cases.

[jason-fox]

The reader is assumed to know that this is how Ant works, but it's easy to forget.

This is the key point.

Either you can assume that the developer understands how ANT works or you need to be explicit. If you feel there is a need to be more explicit, I would suggest keeping the list of <property/> tasks but adding a comment line above the block explaining This sets any properties that have not been set yet - maybe even add in a reference to https://ant.apache.org/manual/Tasks/property.html

<!-- Setting default values of any unset properties ->
<property/>
<property/>
<property/>

This would make the reference explicit but also significantly reduce the number of lines of code which was the intent in the first case. Personally I find it harder to remember the names of the properties if they are spread out so much, and ANT syntax is notoriously verbose already - why make it even more verbose?

So what do you think? Annotate with comments or revert 468a4f2 and leave the other commit?

[eerohele]

I don't think it's too much to expect of anyone who reads or writes Ant build scripts to know that Ant properties are immutable (which, incidentally, is the best thing about Ant).

I'm not sure it's worth writing out the property names twice everywhere on the off chance that someone might not know that.

[jelovirt]

#3297 implements the removal of <condition> guards. If/when this is rebased, only retain the value validation features.

[jason-fox]

@jelovirt - I have isolated the regex-based change and rebased onto the tip of develop

[stale]

This issue has been automatically marked as stale because it has not been updated recently. It will be closed soon if no further activity occurs. Thank you for your contributions.