search

apiaryio / mson-ast

MSON AST Serialization Media Types
MIT License
4 stars 1 forks source link

Address Early Adpoter's feedback #9

Closed zdne closed 9 years ago

zdne commented 9 years ago

This PR addresses all of the points in the feedback as discussed in #8, for @honzajavorek to review & merge

pksunkara commented 9 years ago

lgtm.

honzajavorek commented 9 years ago

Reviewed. Please, search and replace occurrences of "member type" or "member type(s)" or "member types". Otherwise good.

zdne commented 9 years ago

Reviewed. Please, search and replace occurrences of "member type" or "member type(s)" or "member types". Otherwise good.

sigh

honzajavorek commented 9 years ago

It was for sake of consistency. If there are two different definitions for terms "member" and "member type" (even non-formal) let's keep both. Otherwise let's use just one term everywhere.

Or... Maybe I'm just overwhelmed by terms so I can't easily see their original English meanings anymore :-) "member type" is probably just "type of member", no big deal. On Dec 16, 2014 1:55 AM, "Z" notifications@github.com wrote:

Reviewed. Please, search and replace occurrences of "member type" or "member type(s)" or "member types". Otherwise good.

sigh

— Reply to this email directly or view it on GitHub https://github.com/apiaryio/mson-ast/pull/9#issuecomment-67135469.

zdne commented 9 years ago

Guys this wasn't a change "we won't call member type a type anymore", actually I think we are loosing something fundamental here. Let me rework this PR.

pksunkara commented 9 years ago

Sorry about my comment. Yup, this change is only about renaming the data structure. Not the concept.

zdne commented 9 years ago

@pksunkara @honzajavorek nps let me rework it, I will post an update soon

zdne commented 9 years ago

OK, so I have made the changes I am finally happy with, I believe it adds more clarity and a bit simplifies the AST as well. Here is what I have done and why:

1. Member Types & Type Section content

Per MSON AST spec member types are:

  1. Property member type
  2. Value member type

We were sort of forcefully pushing under this term Mixin and One Of types as well as Groups of any of these and tried to call them Member Types and later Members.

I feel this is washing away the original idea in MSON where Member Types are in fact inline anonymous types on their own (unlike mixin or one of). To correct this wrong doing I suggest we use the term "element" (as in Type Section content element, or Type Section's element) for any of the member type, mixin type, one of type or group of thereof.

Note: MSON AST Element has no equivalent in MSON spec because there isn't any! This is important idea behind its name.

2. Overload of "types"

Furthermore since we are talking about Type Definitions, Named Types and Data Structure Types introducing a type property of a heterogenous data structures (Type Section and Member now Element) only adds to the confusion. Hence I have renamed them to class

3. Flatten Mixin

I have flattened the Mixin type from object directly to Type Definition.

In addition to these changes I have added explicit types to every Named Type definition for added clarity (consider Member is an actual keyword and Element is a reserved keyword).

Hope you will like these changes and the AST will be now a bit more clearer.

pksunkara commented 9 years ago

Nitpick: In the pic, the four arrows starting from sample and default can be merged into two arrows. The crossings there look a bit confusing, so merging would look better IMHO.

pksunkara commented 9 years ago

I went over the AST trying to simplify as we talked yesterday, but couldn't any more. So, I am pretty happy with the final result now.

honzajavorek commented 9 years ago

inline anonymous types on their own

:+1:

"element"

The new Element thing is really smart :clap:

Overload of "types"

Class is probably a good choice. The only other word, which came to my mind and would be suitable for these attributes, is kind. It isn't used very often, but it might avoid conotations people already have with classes in programming.

honzajavorek commented 9 years ago

I finished my review. Please, look at the comments and polish nitpicks.

zdne commented 9 years ago

@pksunkara

Nitpick: In the pic, the four arrows starting from sample and default can be merged into two arrows. The crossings there look a bit confusing, so merging would look better IMHO.

I do not see how this can be done.

zdne commented 9 years ago

@honzajavorek @pksunkara comments addressed

pksunkara commented 9 years ago

Just this left and then, this can be merged.

zdne commented 9 years ago

fixed, cheers!