Type hinting & missing "Type Name" sub-objects

honzajavorek commented 9 years ago

I performed two changes:

Added missing Type Name sub-objects (bug).
Added type hints.

Resulting AST should contain all type information. As we agreed in e-mail, if the type information is missing in the original MSON, it should be hinted by parser. The original example was not consistent anyway - e.g. it hinted object on top-level Named Type object although it was not explicitly specified in MSON counterpart, but it did not hinted any types anywhere else.

zdne commented 9 years ago

Added missing Type Name sub-objects (bug).

this looks good, thanks

Added type hints.

I am wondering – is there a reason why you need this while building Boutique – or is this more of a consistency change? Any input on this will help us to round this area of AST. Internally, in the MSON parser, we are holding the information of the implicit type, but we are not exposing it to the public (this AST) at the moment. Question is, whether parser, or its harness (Drafter) should do this? cc @pksunkara

As we agreed in e-mail, if the type information is missing in the original MSON, it should be hinted by parser.

Can you please point me to the email (subject / date)?

The original example was not consistent anyway - e.g. it hinted object on top-level Named Type object although it was not explicitly specified in MSON counterpart, but it did not hinted any types anywhere else.

I believe this was intentional, albeit we are still grappling with the implicit type specification as mentioned above..

For the record (taken from the spec):

Implicit `object`

Any list of Property Member Types collectively MAY define an implied parent object type structure.

2.3.1 Property Member Types

and

A Property Member Type without a Type Definition that contains Nested Member Types implies an object type structure.

4.3 Nested Member Types

Implicit `string`

A Member Type that does not contain Nested Member Types and does not contain a Type Definition implies a string Type Specification.

4.3 Nested Member Types

honzajavorek commented 9 years ago

I have misinterpreted @pksunkara's first answers. I understood that "neither Drafter nor Boutique will have to care about types at all", but from his later response and your response here I see it's not the case and my initial questions in the e-mail conversation regarding type resolution were valid. Your response nicely and clearly answers them, though:

Boutique doesn't have to care about non-primitive types, will get them already resolved/expanded in advance (that wasn't my question - already knew that, just repeating the fact for completeness).
Boutique will contain it's own "type resolution" in means that it will have to resolve the primitive type in cases where it's going to be implicit and not right in the type specification. These implicit types won't be added to the AST by parser or Drafter (might be in Drafter, but that's not so important ATM).
You pointed out the rules how to do it.

Thanks!

In this context, I am going to close this PR and send a new one with the bugfix (name.names) only.

pksunkara commented 9 years ago

@honzajavorek The name.name is not a bug fix. If you look at the AST, the typeSpecification.name is an enum which is represented by a struct in the C++ AST.

@zdne Re implicit type: I think drafter should do this. I would have said snow crash can do this but we are planning on snow crash not having anything extra other than which was written by the user.

honzajavorek commented 9 years ago

@pksunkara Named Type has clearly a property name, which contains Type Name object, which has another property name. And that property name is an enum in means that it can contain listed values and blah blah. I clearly see name.name in the spec.

Regarding implicit types, I agree Drafter could do this. But as the first version of Drafter will be CoffeeScript also, I can implement some kind of implicit type resolution now in Boutique and if we decide it's more suitable for Drafter, it can be easily moved there in the future.

zdne commented 9 years ago

And that property name is an enum in means that it can contain listed values and blah blah. I clearly see name.name in the spec.

@honzajavorek is correct, I have already merged his PR fix. Albeit I am still thinking whether we should not rename one of these two names... Ideas? @honzajavorek @pksunkara

zdne commented 9 years ago

Regarding implicit types, I agree Drafter could do this. But as the first version of Drafter will be CoffeeScript also, I can implement some kind of implicit type resolution now in Boutique and if we decide it's more suitable for Drafter, it can be easily moved there in the future.

I am really concerned whether we should not add resolved implicit types to the last from the Snow Crash already...

honzajavorek commented 9 years ago

@zdne BTW, your extracts from spec are not complete, there is also this rule for arrays:

- address: street, city, state (array)
In this latter case, using a comma-separated list of values, the type (array) is implied and thus MAY be omitted.

Without this, inline arrays would be considered as strings I suppose. From AST point of view, this can be identified only as (pseudocode):

"type is missing" and "size of ...content.valueDefinition.values is greater than zero" means the type is "array"

I only hope it has no race conditions.

pksunkara commented 9 years ago

Damn. I remember discussing with @zdne that an array will always be defined. So, I didn’t add any implicit support for array.

honzajavorek commented 9 years ago

I have experimental implementation and tests with implicit primitive type resolution here.

pksunkara commented 9 years ago

I am really concerned whether we should not add resolved implicit types to the last from the Snow Crash already...

I am okay with this.

pksunkara commented 9 years ago

@honzajavorek is correct, I have already merged his PR fix. Albeit I am still thinking whether we should not rename one of these two names... Ideas? @honzajavorek @pksunkara

Well, why is the Type Name an object in the first place? Can't it be directly an enum? That would solve this issue. If we are unable to do that, I don't want to do anything else. name and name both are very good names. It's the arrangement of the AST which makes it a problem.

pksunkara commented 9 years ago

@zdne

In this latter case, using a comma-separated list of values, the type (array) is implied and thus MAY be omitted.

Actually, I think this is not implemented in the current snow crash.

zdne commented 9 years ago

@pksunkara

Well, why is the Type Name an object in the first place?

please check the discussion here https://github.com/apiaryio/mson-ast/issues/8

zdne commented 9 years ago

Actually, I think this is not implemented in the current snow crash.

Is there a technical reason for it? Are you planning to add it?

pksunkara commented 9 years ago

There was a technical problem for it. I recall discussing with you and we decided that we always make the user say array. I am not 100% sure though.

apiaryio / mson-ast