apiaryio / mson-ast

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

Added Source Map description #5

Closed pksunkara closed 9 years ago

pksunkara commented 9 years ago

Also cleaned up a bit of organization.

pksunkara commented 9 years ago

@zdne Please keep in mind that this is the first draft and even though a lot of thought has been gone into the source maps here, I am open to discussions.

honzajavorek commented 9 years ago

So how is referencing going to work without the top level document?

pksunkara commented 9 years ago

It is not going to work at all. We are currently integrating MSON into snowcrash and so snowcrash will be containing that named types table. I removed this part because I think it should be added when a standalone MSON document parsing is supported which at the first iteration is not going to be.

honzajavorek commented 9 years ago

So this means, that in case I want to work with the AST, after this change

Right? Something like this:

solution-pavan

What I thought is that I would get a list of top-level Named Types and a name of the one I should actually work with, while the others would be only present in the AST for referencing purposes. Construction of such list is obvious in context of standalone MSON; in API Blueprint, you could just put separate definitions of Named Types into one list together, one after another (all MSON definitions from one blueprint would be gathered in single AST). This way I could create my own symbol table and while trying to process the one Named Type (of the given name), I could expand references myself.

(I pondered if there can be clashes in names of Named Types, but there could not be any as referencing has to be unambiguous also within the blueprint document itself.)

So my original proposal was something like this:

solution-honza

However, your proposal says all the referencing and symbol table expansion is going to be done already in parser. Although it's less work for me, I think the AST then doesn't reflect exactly what's written in MSON document. Is it temporary solution just to get the first iteration of MSON done, or is this supposed to survive also for future versions of MSON tooling?

pksunkara commented 9 years ago

No. @honzajavorek You are confusing yourself.

The MSON AST wouldn't contain a list of named types. The named types will not be a part of any AST, either MSON or APIB.

What I thought is that I would get a list of top-level Named Types and a name of the one I should actually work with, while the others would be only present in the AST for referencing purposes.

This is still correct. You will still get a list of named types. They are not going to be exported anywhere outside. There is a big difference here about what boutique is going to get and what the end user is going to get.

zdne commented 9 years ago

@honzajavorek @pksunkara

  • I get only one top-level named type at most. Top-level structure of the AST is a single Named Type object.
  • I don't have to care at all about Referencing and Mixins, because they'll be just pre-expanded for me in form of direct type definition objects.

Both is correct.


The input of a boutique should be one fully-expanded MSON tree and a media type to transform it into. Nothing else as far as I can see.

honzajavorek commented 9 years ago

(originally I wanted to use word "tooling" and to avoid saying "Boutique" as it's private and this is a public repo, but OK)

No. @honzajavorek You are confusing yourself.

I had an email discussion with @zdne on this topic and we agreed on sth. Now I realized we both actually thought sth else, so I just presented what I originally meant.

This is still correct. You will still get a list of named types. ...

I'm sorry, I don't understand much this your paragraph. From my two presented approaches, you can't get both. Also, it's in contradiction with your actual change and with @zdne's comment.

Both is correct.

Thank you. So when writing my comment above, I had understood your proposed solution right.

The input of a boutique...

Then I'm going to change the public interface and some parts of Boutique once again, but fortunately this isn't a big deal and as I said above, at the end of the day it's a lot of less work for me in Boutique.

Thank you both for your prompt replies! While I now understand the change, I'm going to obey it and I know what to do with it, I'm still unsure whether it is the approach we should go in long-term. One of my questions is still unanswered:

...I think the AST then doesn't reflect exactly what's written in MSON document. Is it temporary solution just to get the first iteration of MSON done, or is this supposed to survive also for future versions of MSON tooling?

pksunkara commented 9 years ago

I think the AST then doesn't reflect exactly what's written in MSON document. Is it temporary solution just to get the first iteration of MSON done, or is this supposed to survive also for future versions of MSON tooling?

If I understand your question correctly, the answer is that the current MSON AST supports both the exact form of MSON as written by the user and also the expanded MSON which was meant by the user.

honzajavorek commented 9 years ago

Thank you! That answers my question. Also @zdne helped me to understand the current state of things, especially what is going to be Drafter's responsibility. Thank you, guys! :balloon:

honzajavorek commented 9 years ago

Today I scanned over all the changes (also the sourcemap part) and it seems OK to me. Merge? @zdne

pksunkara commented 9 years ago

@zdne Need to have a small discussion about this before you merge. Thanks.

zdne commented 9 years ago

@pksunkara please poke me when this is updated and good to merge

pksunkara commented 9 years ago

@zdne This is updated.

zdne commented 9 years ago

@pksunkara this PR looks unfinished. Also please revert the formatting change (new lines after headers)

zdne commented 9 years ago

Done with the review.

pksunkara commented 9 years ago

Updated.

Pavan Kumar Sunkara

On 23 December 2014 at 9:16:57 pm, Z (notifications@github.com) wrote:

Done with the review.


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