search

common-workflow-language / cwltool

Common Workflow Language reference implementation
https://cwltool.readthedocs.io/
Apache License 2.0
332 stars 229 forks source link

cwltool allows duplicate baseCommands #562

Open mr-c opened 6 years ago

mr-c commented 6 years ago

Expected Behavior

Tell us what should happen

Validation error

Actual Behavior

Tell us what happens instead

No validation error

Workflow Code

cwltool https://raw.githubusercontent.com/common-workflow-language/common-workflow-language/62fac9316653f4de1cf34c7e4020ca09434f8889/v1.0/v1.0/inline-js.cwl --help

Your Environment

itajaja commented 6 years ago

I don't think this is a problem with CWL tbh. that's just how json/yaml is spec'ed, last values in a map take precedence. For example:

import json
json.loads('{"a": 1, "a": 2}')
>>> {'a': 2}

there is nothing abnormal with it

mr-c commented 6 years ago

@itajaja While I agree that ignoring duplicate keys is not abnormal from a technical perspective, it is not good from a user experience perspective.

LourensVeen commented 6 years ago

Actually, YAML considers duplicate keys an error. From http://www.yaml.org/spec/1.2/spec.html:

JSON's RFC4627 requires that mappings keys merely “SHOULD” be unique, while YAML insists they “MUST” be. Technically, YAML therefore complies with the JSON spec, choosing to treat duplicates as an error. In practice, since JSON is silent on the semantics of such duplicates, the only portable JSON files are those with unique keys, which are therefore valid YAML files.

and

Since YAML mappings require key uniqueness, representations must include a mechanism for testing the equality of nodes. This is non-trivial since YAML allows various ways to format scalar content. For example, the integer eleven can be written as “0o13” (octal) or “0xB” (hexadecimal). If both notations are used as keys in the same mapping, only a YAML processor which recognizes integer formats would correctly flag the duplicate key as an error.

Perhaps this should be considered a bug in ruamel.yaml? It should throw an exception, rather than silently ignoring all but the last entry.

mr-c commented 6 years ago

http://yaml.readthedocs.io/en/latest/api.html?highlight=duplicate#duplicate-keys

mr-c commented 6 years ago

To be clear, once we upgrade to the newer ruamel.yaml version then duplicated keys will be caught