Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add info field to Code block #22

Merged
merged 2 commits into from
Aug 1, 2018

Conversation

arobase-che
Copy link
Contributor

Hi :)

It's just an update of the Code's description. The main modification is the info string tag.

Double check my english please. 🙏

💕

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Looks very nice! One thing I’m wondering is why not just use info instead of infoString?

readme.md Outdated Show resolved Hide resolved
@arobase-che
Copy link
Contributor Author

:)

No reason, it's as you wish

@Hypercubed Yeah I was thinking something along those lines, a code.infoString being the whole thing, lang being the first “word”? It’s breaking, but more fixing.

So i take infoString, in first place i taked "info-string" and it's definitively ugly.

@Hypercubed
Copy link

I would think that since the common mark spec refers to it as the info string the property should be infoString... but it doesn't bother me either way.

@arobase-che
Copy link
Contributor Author

For the language, it's "lang" so for the info string, the tag "info" doesn't seen weird.

For the request, it's done (for the attribute's name too btw)

@wooorm
Copy link
Member

wooorm commented Jul 29, 2018

Yeah I think I prefer info over infoString. Feels a bit weird to add the string prefix, whether commonmark does it or not!

@wooorm
Copy link
Member

wooorm commented Jul 29, 2018

Say we have this:

```js no-highlight
fn()
```

And we have the node for that code:

{
  type: 'code',
  info: 'js no-highlight',
  lang: 'js',
  value: 'fn()'
}

...what if someone changed node.lang = 'c' or something else?

  • Should info hold the whole string, and lang a part of it?
  • Should lang hold the first word, and info other information after it (no-highlight)?

@arobase-che
Copy link
Contributor Author

arobase-che commented Jul 29, 2018

I think that info should be the whole string. The main reason is that the definition of info string from GMF and CommonMark is the whole string. An other is that it's common to specify the language as the first word but it's not specified.

Spec :

The first word of the info string is typically used to specify the language

But as you notify, it can lead to inconsistent states.

PS: Oups ! I miss clicked on close and comment ... >_<"

@arobase-che arobase-che reopened this Jul 29, 2018
@wooorm
Copy link
Member

wooorm commented Jul 29, 2018

I’d rather not specify something that could create inconsistencies, and IMO it makes sense to see:

~~~ ruby startline=3 $%@#$
~~~

to

{
  type: 'code',
  lang: 'ruby',
  info: 'startline=3 $%@#$'
}

...note, btw, I don’t think commonmark specifies what a “word” is in:

The first word of the info string is typically used to specify the language of the code sample, and rendered in the class attribute of the code tag. However, this spec does not mandate any particular treatment of the info string.

@arobase-che
Copy link
Contributor Author

arobase-che commented Jul 30, 2018

:(
Ok

So it's more complicated. Now it need specifications 😢

So ... here we go !

~~~js info
~~~

lang='js' info='info'

~~~js      info    
Trailing spaces before and after the info string are skipped
~~~

lang='js' info='info'

~~~js
No info string is null
~~~

lang='js' info=null

Is it ok like that ?

Edit: Easy spec, i don't want it to be too complicated. Can't handle complicated things.

@wooorm
Copy link
Member

wooorm commented Jul 31, 2018

Yeah I think this is fine! I don’t believe we should care about spacing between lang and info. I believe those could be stripped away. We may need to work out what a “word” means though. What do you think?

@ChristianMurphy Any thoughts?

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 31, 2018

~~~js      info    
Trailing spaces before and after the info string are skipped
~~~

So it would trim external space but not internal?
E.G.

~~~ js    something    that  has      random  spacing
~~~

would parse to

{
  "info": "something    that  has      random  spacing"
}

rather than

{
  "info": "something that has random spacing"
}

?

lang='js' info=null
Is it ok like that?

Sounds good

@arobase-che
Copy link
Contributor Author

Actually here, we don't really care about words but the most important is that it can handle programming language names.

That to Wikipedia, programming languages list.

In the code i defined it by every char until a white space (\s) or a curly bracket (but we may add every kind of bracket).

Yeap, it will trim extrernal but not internal spaces. Or maybe no trim at all ? The choice is yours.

@wooorm
Copy link
Member

wooorm commented Jul 31, 2018

@ChristianMurphy Yup, that’s what I’m thinking of!
@arobase-che I feel like white-space should indeed be the border!

@arobase-che
Copy link
Contributor Author

arobase-che commented Jul 31, 2018

@wooorm ? Border ? An example please ? Only white-space you mean ?

Triming internal spaces can be tricky.

~~~js value="   i    want   it    spaced    "
~~~

@wooorm
Copy link
Member

wooorm commented Jul 31, 2018

@arobase-che I mean the “border” between lang and info. And I believe internal white-space should be kept as is. Just the “border” should be removed.

@arobase-che
Copy link
Contributor Author

arobase-che commented Jul 31, 2018

Ok so :

~~~ golang info
~~~

lang='golang' info='info'

But :

~~~ go{info}
~~~

lang='go{info}' info=null

It become a problem with that :

~~~ go( this is a comment )
~~~

:(

Sorry if I don't understand you. We keep all characters until the first white-space (white-space excluded, so the white-space is the border no ?). The rest of the text is the info attributes and we trim it out (so the border is removed ?)

I believe too that internal white-space should be kept.

@ChristianMurphy
Copy link
Member

~~~ go( this is a comment )
~~~

Which spec supports comments in the info string?
Commonmark makes no mention of it. https://spec.commonmark.org/0.28/#example-112
Does one of the other standards support it?

@wooorm
Copy link
Member

wooorm commented Jul 31, 2018

Yes, I believe that:

```js something
```

```js       something
```

```go( something)
```

```go( s om    ethi   ng)
```

Should turn into: "js", "something", "js", "something", "go(", "something)", "go(", "s·om····ethi···ng)"

@arobase-che
Copy link
Contributor Author

arobase-che commented Jul 31, 2018

@ChristianMurphy None i think. It's about the user's expected result i think. Some may want to use Kramdown Attribute lists.

No language has brackets in its name. So why make a error consciously ?

I will code it in any case but ... :s

@ChristianMurphy
Copy link
Member

None i think. It's about the user's expected result i think

Maybe it could be an option or a plugin that could be enabled.
As a user of remark myself, I would find output that doesn't match commonmark unexpected.

@arobase-che
Copy link
Contributor Author

Ok, makes sense. Even if i think that CommonMark is wrong.

See you soon people ^^

@arobase-che
Copy link
Contributor Author

I updated the description and the PR on remark. ^^

But travis still buging 😢 remarkjs/remark#345

@ChristianMurphy
Copy link
Member

It probably started during the minor Travis CI outage.
Restarted bugging travis instance, it passed this time.

@arobase-che
Copy link
Contributor Author

arobase-che commented Aug 1, 2018

\o/

Thank you 😄

@ChristianMurphy ChristianMurphy merged commit 8453ad9 into syntax-tree:master Aug 1, 2018
wooorm pushed a commit that referenced this pull request Aug 20, 2018
Closes GH-22.

Reviewed-by: Christian Murphy <Christian.Murphy.42@gmail.com>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>
wooorm added a commit that referenced this pull request Aug 20, 2018
Related to GH-22.
Related to d797ea3.
wooorm added a commit to remarkjs/remark that referenced this pull request Oct 6, 2018
Previously, the info string of a code block, was not parsed at all, and was stored in `lang`
on `code` nodes.
Now, the first “word” of the info string is stored in `lang`, whereas the rest is stored in
`meta`.

Related to syntax-tree/mdast#22 (PR)
Closes GH-342 (issue)
Closes GH-345 (PR)

Reviewed-by: Titus Wormer <tituswormer@gmail.com> 
Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com> 

Co-authored-by: Titus Wormer <tituswormer@gmail.com>
@wooorm wooorm changed the title Update description of Code Add info field to Code block Aug 12, 2019
@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have labels Aug 12, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

4 participants