Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

Discussion about #131 #138

Open
BlockLucas opened this issue Aug 10, 2020 · 4 comments
Open

Discussion about #131 #138

BlockLucas opened this issue Aug 10, 2020 · 4 comments

Comments

@BlockLucas
Copy link

Hi!

We have some observations:

  1. Types with question marks are invalid or at least not defined, they have a definition of its usage on properties (or Operations), a corner case for this could be something like this:
types:
  TypeWithTrailing?:
    type: string
  TypeWithTrailing:
    type: int
  TestObject:
    type: object
    properties:
      notnilable: TypeWithTrailing?

https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/EdgeCases/type-with-trailing-question-mark/invalid-notnilable-missing-value.raml
https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/EdgeCases/type-with-trailing-question-mark/valid-nilables-with-values.raml
https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/EdgeCases/type-with-trailing-question-mark/valid.raml

  1. Library used inside a NamedExample is invalid, as "uses" key is being taken as the name of the example. Another thing for this is that examples should be static, having an annotation that is being resolved by the example itself (hence validating its own definition) doesn't seem right.
    https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/Libraries/used-by-namedexample

  2. That long key that is being used in the enum, is not valid against the schema, also, in line 12, there is a typo (should be "string" instead of "strings")
    https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/Root/yaml-12-compliancy/valid.raml

  3. There is no union for custom facets, this case is fullfilling a custom facets value that is only present in one of the elements: https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/Types/Type Expressions/union-with-facets/valid-custom-facet.raml
    Similar to the earlier case, https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/Types/Type Expressions/union-with-facets/valid-supported-facet.raml is fulfilling a value for a facet that is not defined.

  4. Default types (https://github.com/raml-org/raml-spec/blob/master/versions/raml-10/raml-10.md/#determine-default-types) doesn't explain what to do with this case.
    Even if we take "minimum" to infer the type, this facet is present in Number & Integer types.
    https://github.com/raml-org/raml-tck/blob/i130_add_raml101_tests/tests/raml-1.0/Types/determine-default-types/valid-number.raml

@postatum
Copy link
Contributor

Hi @BlockLucas
Thanks for taking time to look into the PR!

  1. In these tests, question mark is part of the type name. See Clarify nilable property alternative syntax raml-spec#708 (comment). Also RAML 1.0 spec doesn't pose any restrictions on type names. Though I agree that this case is confusing. I think the usage of question marks in types names should be either clarified in the spec, or disallowed and the tests removed.

  2. uses in fragments will be clarified in the RAML 1.0.1 (Clarify that libraries may be used in any type of fragment raml-spec#718 (comment)). I have nothing to say on your second point though.

  3. Good catch. Fixed (16bf431).

  4. I don't understand what you mean by "There is no union for custom facets", please clarify. These tests are supposed to address Clarify use of type-specific facets in a union type raml-spec#711 which clarifies the usage of facets in unions. Explanation of both tests you've mentioned is in the refered raml-spec issue.

  5. The idea behind this test is: It's possible to determide the type of this property since only numbers can have floating point values. I may have went too far to assume floating point in minimum also means the property will have floating point values.

@postatum
Copy link
Contributor

@jstoiko please share your opinion on these issues.

@jstoiko
Copy link
Contributor

jstoiko commented Aug 18, 2020

Hi @BlockLucas, thanks for the input.

  1. As far as I can tell keys with trailing ? are valid as neither YAML 1.2 nor RAML 1.0 forbids it - We did add a clarification to that effect though (i.e. The use of that equivalent alternative syntax SHALL be restricted to ... references to user-defined types ... ) but feel free to suggest more clarification if you think it's necessary. If you want to discuss this issue further, I invite you to leave a comment in: Clarify nilable property alternative syntax raml-spec#708
  2. We did some research and the original intent was to be able to refer to libraries from within any typed fragments. The clarification we came-up with reflects that. Feel free to discuss the issue further in Clarify that libraries may be used in any type of fragment raml-spec#718
  3. 👌
  4. Did Clarify use of type-specific facets in a union type raml-spec#711 clarify the rationale behind this one? If not, feel free to leave a comment in that issue.
  5. The clarification says: If, and only if, a type declaration contains a facet that is unique to that type, then its default type is inferred to be the only one with support for the facet being used. and I think to your point, minimum does not meet that definition since it may be present in both integer and number types. @postatum: I don't think the value should be defining the type, otherwise we wouldn't need types at all... no? Maybe you have a different rationale that you'd want to discuss. If not, can you fix this test? I think a facet like fileTypes (inferring type: file) would be more appropriate in this case.

postatum added a commit that referenced this issue Aug 19, 2020
@postatum
Copy link
Contributor

I don't think the value should be defining the type, otherwise we wouldn't need types at all... no? Maybe you have a different rationale that you'd want to discuss. If not, can you fix this test? I think a facet like fileTypes (inferring type: file) would be more appropriate in this case.

Done. I deleted number and integer tests. Tests for other types are already there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants