Early adopters' feedback on MSON AST

[honzajavorek]

• [x] 1. Rename name.name object? (here, here (private) and elsewhere)

  Result: Type Name will be enum not an object therefore we get rid of one name property

• [x] 2. Provide hints for implicit primitive types already in parser? (here)

  Result: We will not expose this information at the moment but may do so in the future.

• [x] 3. Renaming MemberType to just Member? (here)

  Result: Rename MemberType to Member

  The answer is more complex and discussed in https://github.com/apiaryio/mson-ast/pull/9

• [x] 4. Unite NamedType. base.typeSpecification with ValueMebmer ... typeDefinition.typeSpecification

  Result: Rename base to typeDefinition

---

Update:

@zdne has edited this post to:

• use checkmarks to denote already resolved points
• added point No.4

---

Let's add more and more as we use the AST and let's discuss these questions in comments. @zdne @pksunkara

[honzajavorek]

FYI, when implementing implicit type resolution as described here, I've run into ambiguity of what does contains Nested Member Types mean. It can mean:

1. There are existing nested members.
2. There are existing "member" sections.

The difference becomes visible in case there could be an empty "member" section. In that case, counting nested members would result in zero occurrences, but presence of the section could in fact indicate that the parent object is "object", albeit empty one. Thus, second option makes more sense to me, so I used that approach.

[honzajavorek]

I wonder if we could unite these two:

• Named Type . base . typeSpecification
• Value Member ... typeDefinition . typeSpecification

(base vs. typeDefinition)

[pksunkara]

There are no empty member sections.

[pksunkara]

What do you mean by ‘unite’? They are using the same Object TypeSpecification.

[honzajavorek]

Empty member sections - probably I'm just overthinking it and counting with race conditions, which can't happen thanks to parser.

By uniting I meant to name the same things the same way. base and typeDefinition are the same things, but they have different names in the AST, which isn't the case for anything else there AFAIK.

[pksunkara]

The different names for them is intentional. They represent different things in a NamedType and ValueMember

[zdne]

Ad 3.:

Renaming MemberType to just Member?

I concur. Let's do it unless anyone of you has any objections.

[zdne]

Ad 2.:

Provide hints for implicit primitive types already in parser

This is mainly for you @honzajavorek – do you feel that duplicating this information would help subsequent tooling writers (and you in the first place?)

[zdne]

Ad 4.:

By uniting I meant to name the same things the same way. base and typeDefinition are the same things, but they have different names in the AST, which isn't the case for anything else there AFAIK.

@honzajavorek I am not certain what you want to unite? The name of the properties in the AST? Let's say instead of base use typeDefinition?

[zdne]

Ad 1.:

Rename name.name object

I suggest to rename Type Name's name property to just type. Agree?

[zdne]

Ad 4.:

If I understand @honzajavorek correctly I do concur and suggest to rename base to typeDefinition including the C++ AST for two reasons:

1. to avoid clash with the MSON definition of word "base type"
2. to be consistent with Value Member since these two properties (base and typeDefinition) are essentially the same

Thoughts?

[zdne]

@honzajavorek sorry for modifying your original post, I think this way it will help us to stay clear on what is being discussed. When adding a new point add it to the OP. Hope its OK. Thanks.

[honzajavorek]

@zdne thanks for modifying, much better now :)

1

My opinion: Let's use type.

Reasoning: type sounds good and it's already used in the AST on different places for similar things IMHO. I do concur.

2

My opinion: Let's see how it plays together first and let's decide later.

Reasoning: I'd leave this in Boutique at the moment. We don't have to answer this immediately and it's already implemented in Boutique (https://github.com/apiaryio/boutique/pull/22). In the future we can move this to Drafter if we think it's more suitable place, leave it in Boutique - it's just copy-paste of CoffeeScript between two repos. Or, we can decide we want it in parser. Moving to parser would mean rewriting it to C++, but C++ is going to eat both Boutique and Drafer in the future anyway, so... from my POV this is no big deal.

3

My opinion: Let's use Member.

Reasoning: I re-read the spec and I concur. Just "Member" fits in the context of all the other names I can see there (Value Member, Property Member, Members, ...).

4

My opinion: Let's use single word. No strong opinions on which one exactly.

Reasoning: You understood me right, it was just about naming. It came to my mind exactly because of

to be consistent with Value Member since these two properties (base and typeDefinition) are essentially the same

I like base a bit more though, because it's shorter and nicer, but I don't know whether it's a good name for "Value Member's type" and also there is the clash with word "base type" you mentioned above. No strong opinions on which one to choose.

[pksunkara]

4

base is definitely going to be more confusing. I would like to go with typeDefinition.

1

If you look at the C++ AST, even the name type is going to be quite confusing.

    /** Base or named type's name */
    struct TypeName {
        /** EITHER Base type's value */
        BaseTypeName name;

        /** OR Named type's identifier */
        Symbol symbol;
    };

As I said in another thread, we don't actually need TypeName to be an object (even taking into account future changes to it such as referencing). So, unless there's something I don't know, I would like it to be an enum.

[zdne]

Ad 1.:

Reasoning: type sounds good and it's already used in the AST on different places for similar things IMHO. I do concur.

Yup.

As I said in another thread, we don't actually need TypeName to be an object (even taking into account future changes to it such as referencing). So, unless there's something I don't know, I would like it to be an enum.

On a second thought, and given the need to make the AST as simple possible, I agree with @pksunkara, let's make it more flat and use an enum (and hope we won't need to change this in a future). Seems like Symbol may take care of it of any possible future extension when it comes to referencing.

[zdne]

Ad 2.:

Moving to parser would mean rewriting it to C++, but C++ is going to eat both Boutique and Drafer in the future anyway, so... from my POV this is no big deal.

Parser has already this information in its AST. So the question is really whether we should expose it or not, since pretty much everybody needs it.

But let's skip it for now then.

[zdne]

Ad 4.:

lets go with typeDefinition reserve the word base for MSON base types object, string etc.

[zdne]

@honzajavorek @pksunkara I have updated the OP & reflected the changes in this PR https://github.com/apiaryio/mson-ast/pull/9

@pksunkara please let @honzajavorek to review & merge. Once merged @pksunkara to update Snow Crash accordingly (if applicable), after that @zdne to update Protagonist experimental

[honzajavorek]

I agree with flattening the name.name.

Parser has already this information in its AST. So the question is really whether we should expose it or not, since pretty much everybody needs it.

While reading this I realized we can expose these types and at the same time, we can keep the principle that you should be able to exactly build the MSON document from it's AST. We just need to add a new attribute where the implicit primitive type is going to live, side by side with what we have now. I would like such solution.

[pksunkara]

Actually, we don’t need to add anything new. We can just add new values to the TypeName such as implicitObject, implicitArray, implicitString.

[honzajavorek]

we don’t need to add anything new. We can just add new

I'm reading these words again and again and... well, let's just say we agree ;-)

[honzajavorek]

Ha! I think I got what you wanted to say. However, I don't like it very much - introducing new types like that would have much larger impact on the spec (and everything around) than a new attribute to carry implicit type with values from existing range of types. Makes much more sense to me to use types we have and append a new attribute with implicit type, which can (but doesn't have to) be used by subsequent tooling.

[zdne]

@honzajavorek @pksunkara introducing a property for deduced ancestors – it should be a new property with its own enum, not adding elements to the Type Name enum.

[zdne]

please let's continue this discussion over the PR #9

[zdne]

Note I have just released protagonist experimental v0.18.2 that includes the changes to AST as discussed here and proposed in #9

If you are building a tool on top of the MSON AST please make sure you are using this version and not the older one. (cc @honzajavorek @danielgtaylor)

If you have a further feedback on how the AST can be improved please open a new Issue or a PR. Thanks!