Skip to content

Source file information RFC#12

Merged
pksunkara merged 4 commits intomasterfrom
zdne/file-source-map
Sep 7, 2015
Merged

Source file information RFC#12
pksunkara merged 4 commits intomasterfrom
zdne/file-source-map

Conversation

@zdne
Copy link
Copy Markdown
Member

@zdne zdne commented Aug 24, 2015

In order to support multiple files in the future we need to carry information about the source file in the source map. This RFC suggest prepare for this by changing the current definition of the Source Map element.

Comment thread text/0000-file-source-map.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This RFC SHOULD be..."

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Aug 24, 2015

@zdne One question I have is, will there every be a situation where a source map element may need to broken apart for for multiple files? In other words, could one element be derived from two different files, requires more complex source map?

Hope that makes sense.

Comment thread text/0000-file-source-map.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

@pksunkara
Copy link
Copy Markdown
Contributor

@smizell has a good question. Otherwise this is good to go for me.

Z added 2 commits August 25, 2015 17:53
Fix typo, indentation and add clarification on using source map
“unperfected”.
@pksunkara
Copy link
Copy Markdown
Contributor

Looks good. Still need to answer the following.

One question I have is, will there every be a situation where a source map element may need to broken apart for for multiple files? In other words, could one element be derived from two different files, requires more complex source map?

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Aug 25, 2015

One question I have is, will there every be a situation where a source map element may need to broken apart for for multiple files? In other words, could one element be derived from two different files, requires more complex source map?

That makes very much of the sense!

Theoretically this can happen albeit I do not see it at the moment. In such a case the source map should be obviously an array.

Are you suggesting the sourceMap attribute should be an array of sourceMap elements? (That makes sense to me).

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Aug 25, 2015

Alternatively a sourceMap element content could be the map itself AND information about the file.

Something like

{
       "element": "sourceMap",
       "attributes": {},
       "content": [
           {
               "source": {},
               "map": [[4, 12], [20, 12]]
           }
       ]
   }

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Aug 26, 2015

@smizell @pksunkara guys, I've updated the document and added some thoughts here. Please review & comment. Thanks!

@pksunkara
Copy link
Copy Markdown
Contributor

I like the second proposal since it is more compact, but I don't like that the content is not an element in that example

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be possible that an element might span across multiple files?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, sorry didn't see that. Was only looking in file view.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great minds think alike?

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Aug 27, 2015

@pksunkara @smizell bump 😜

@pksunkara
Copy link
Copy Markdown
Contributor

@zdne I already reviewed your changes here and also offered my opinion on your proposal for broken up source map over multiple files.

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Aug 27, 2015

@zdne Thanks for putting up with my neglect of this :)

My thought here is, you need each source map to be able to be associated with a file, but you may need to multiple files associated with a specific element. This means an element should be able to have multiple source maps. I'm not sure either proposal here addresses that.

BUT if that is not a necessity short term (either too complex or premature optimizing) and you are not interested in addressing multiple files for one element, I'd vote for your proposal as it allows you to have one-to-one with source file and source map. This means you can add multiple source maps later if you want.

{
  "element": "...",
  "attributes": {
    "sourceMap": [
      {
        "element": "sourceMap",
        "attributes": {
          "contentType": "text/vnd.apiblueprint+markdown",
          "href": "apiary.apib1"
        },
        "content": [[4, 12], [20, 12]]
      },
      {
        "element": "sourceMap",
        "attributes": {
          "contentType": "text/vnd.apiblueprint+markdown",
          "href": "apiary.apib2"
        },
        "content": [[4, 12], [20, 12]]
      }
    ]
  }
}

So two things:

  1. I'll let you determine if you want the multiple files now.
  2. I think either way, I'm for your main proposal

@kylef
Copy link
Copy Markdown
Member

kylef commented Aug 28, 2015

@smizell I like the proposal, I think we should keep both the file and the content offsets together in one element that splitting them across two.

@pksunkara
Copy link
Copy Markdown
Contributor

My thought here is, you need each source map to be able to be associated with a file, but you may need to multiple files associated with a specific element. This means an element should be able to have multiple source maps. I'm not sure either proposal here addresses that.

@smizell I am sorry but I thought @zdne's proposals above (which is the same as your example in the above comment) solves this. Maybe I am misunderstanding. Can you explain a bit where his proposal is lacking/failing?

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Aug 28, 2015

Maybe I'm reading it wrong, but his current proposal does not address multiple files. The sourceMap property in attributes is just one sourceMap element. My example has sourceMap being an array of elements.

@pksunkara
Copy link
Copy Markdown
Contributor

Oh. I was more referring to the last line in this comment. @zdne wanted to make attributes.sourceMap an array of sourceMap elements.

Though, I wonder if attributes.sourceMaps is a better key name.

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Aug 28, 2015

Ah thank you. I did miss that. That may be the way to go, though should it always be an array? Could it be a sourceMap element or array of sourceMap elements?

Simplest is always array. Then you could change the property name as you've said.

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Aug 29, 2015

So, the current question is:

  1. Do we stick with current main proposal of keeping sourceMap and use attributes for file, or
  2. Make the sourceMap attributes property an array of sourceMap elements

What do you think, @zdne

@danielgtaylor
Copy link
Copy Markdown
Contributor

I like the array of source maps idea. I also like the name attributes.sourceMaps since it implies I'll have to potentially handle more than one. It's less surprising for the implementer.

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Sep 1, 2015

@danielgtaylor Sounds good!

Will wait on @zdne here thoughts between:

  1. Use sourceMap attribute where value is sourceMap element.
  2. Use sourceMaps attribute where value is an array of sourceMap

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Sep 3, 2015

@smizell sourceMap it is. I've updated the RFC – see 3102715

Please review & let me know!

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Sep 3, 2015

@zdne I'm ready to merge this. One thing though I want to be sure of is that you have a singular name sourceMap for the array. Is that intended. I'm OK with it, just wanted to make sure.

@pksunkara
Copy link
Copy Markdown
Contributor

I thought we wanted to use sourceMaps name if we are going the array way.

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Sep 4, 2015

@smizell @pksunkara this use of singular is intentional, yes. It is a source map of the element (possibly composed of multiple source maps).

I do not like the names of properties hinting the type of its values (Hungarian notation). I prefer using plural when it makes sense from semantical standpoint e.g. "items" in a shopping cart. We were discussing this with @danielgtaylor on unrelated issue about "copy" vs. "copy texts" (singular vs. plural): If you have a collection of possibly unrelated copy information then I'd suggest to use plural form but contrary to it if the property represents a copy information about the element (regardless of the fact it is composed from an object or array or whatnot) then it would be singular.

Does it clarify?

In other words I want to avoid following:

  • array -> plural (sourceMap -> sourceMaps)
  • object -> object suffix (sourceMap -> sourceMapsObject)

etc.

@smizell
Copy link
Copy Markdown
Contributor

smizell commented Sep 4, 2015

I don't want to carry this out too long, but did want to point out a couple of things. Note: I am really fine personally either way.

  1. We have used this pattern of "plural meaning a collection" in other places (see classes)
  2. We are using this as an interface as well element.meta.classes.first()

I get the reasoning of not making plural, though. Proceed as you see fit.

@pksunkara
Copy link
Copy Markdown
Contributor

We have used this pattern of "plural meaning a collection" in other places (see classes)
We are using this as an interface as well element.meta.classes.first()

This is my main concern. We are using the pattern elsewhere. I have no problem in merging this currently. Either way is fine with me.

@zdne
Copy link
Copy Markdown
Member Author

zdne commented Sep 7, 2015

We have used this pattern of "plural meaning a collection" in other places (see classes)
We are using this as an interface as well element.meta.classes.first()
I get the reasoning of not making plural, though. Proceed as you see fit.

@pksunkara @smizell

I didn't really want to get into this here BUT I've never liked the classes.

There, the property clearly defines what a "class" of the element is. Regardless of the fact the content of it is an array of strings. All the strings together define the compound class of the element. I believe the main motivation there was not the fact to express the collection but avoid the clashes with some programming languages. As such I wasn't against it because the use of plural has a technical reasoning not semantical.

pksunkara added a commit that referenced this pull request Sep 7, 2015
@pksunkara pksunkara merged commit d760a28 into master Sep 7, 2015
@pksunkara pksunkara deleted the zdne/file-source-map branch September 7, 2015 13:58
@smizell
Copy link
Copy Markdown
Contributor

smizell commented Sep 8, 2015

@zdne Makes sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants