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

Idea: Support CSS #152

Closed
tomek-he-him opened this issue Feb 12, 2015 · 19 comments
Closed

Idea: Support CSS #152

tomek-he-him opened this issue Feb 12, 2015 · 19 comments

Comments

@tomek-he-him
Copy link
Contributor

I know dox isn't designed to document CSS. But in the world of stylesheets there is no tool as flexible, generic, unopinionated and fabulous as dox.

AFAIK dox only parses comments. CSS comments are compatible with JS comments. Should work out of the box.

So we already decided to give it a try. Everything worked like a charm. Well, almost everything.

We've encountered only one pitfall – an escaped apostrophe in a selector (\'). When dox encounters one of these, it truncates all following comments.

So this works great:

/**
 * A simple boring class
 */
.a-simple-boring-class {}

/**
 * Another one
 */
.another-one {}
$ dox --debug < this
[ { tags: [],
    description:
     { full: '<p>A simple boring class</p>',
       summary: '<p>A simple boring class</p>',
       body: '' },
    isPrivate: false,
    isConstructor: false,
    isEvent: false,
    ignore: false,
    line: 1,
    codeStart: 4,
    code: '.a-simple-boring-class {}',
    ctx: undefined },
  { tags: [],
    description:
     { full: '<p>Another one</p>',
       summary: '<p>Another one</p>',
       body: '' },
    isPrivate: false,
    isConstructor: false,
    isEvent: false,
    ignore: false,
    line: 6,
    codeStart: 9,
    code: '.another-one {}',
    ctx: undefined } ]

Whereas that fails:

/**
 * This class's got an apostrophe
 */
.this-class\'s-got-an-apostrophe {}

/**
 * This will get truncated
 */
.this-will-get-truncated {}
$ dox --debug < that
[ { tags: [],
    description:
     { full: '<p>This class&#39;s got an apostrophe</p>',
       summary: '<p>This class&#39;s got an apostrophe</p>',
       body: '' },
    isPrivate: false,
    isConstructor: false,
    isEvent: false,
    ignore: false,
    line: 1,
    codeStart: 4,
    code: '.this-class\\\'s-got-an-apostrophe {}\n\n/**\n * Another one\n */\n.another-one {}',
    ctx: undefined } ]

You can read more about escaped characters in this article by Mathias Bynens.

@tomek-he-him
Copy link
Contributor Author

This might be directly connected with #149.

This line has an apostrophe: https://github.com/trusktr/readem/blob/d51f9e899702318526bd7322e10bb1e8fbd671f9/bin/read'em.js#L51

@trusktr
Copy link
Contributor

trusktr commented Feb 16, 2015

@tomekwi Confirmed, it's a \' causing the problem. In my case it not line 51, but line 47.

@tomek-he-him
Copy link
Contributor Author

@tj @evindor what's your view on this? I'm about to publish a LESS-dox adapter – but wouldn't want to do it if it's not an intended use for dox.

@Twipped
Copy link
Collaborator

Twipped commented Mar 18, 2015

@tomekwi TJ made me the maintainer this morning. I made commit 55d2b92 today which addressed an issue with dox falsely picking up quotes in comments as being string literals. Could you please check against master and let me know if you're still having an issue?

@tomek-he-him
Copy link
Contributor Author

Thank you :) I'll do that tomorrow as soon as I'm in my office.

@tomek-he-him
Copy link
Contributor Author

Dammit, forgot about that. Now I'm on leave so I no longer have access to the office. But I've checked against the original CSS example.

This still fails:

$ cat <<———— | dox
/**
 * This class's got an apostrophe
 */
.this-class\'s-got-an-apostrophe {}

/**
 * This will get truncated
 */
.this-will-get-truncated {}
————
[
  {
    "tags": [],
    "description": {
      "full": "<p>This class&#39;s got an apostrophe</p>",
      "summary": "<p>This class&#39;s got an apostrophe</p>",
      "body": ""
    },
    "isPrivate": false,
    "isConstructor": false,
    "isClass": false,
    "isEvent": false,
    "ignore": false,
    "line": 1,
    "codeStart": 4,
    "code": ".this-class\\'s-got-an-apostrophe {}\n\n/**\n * This will get truncated\n */\n.this-will-get-truncated {}"
  }
]

@tomek-he-him
Copy link
Contributor Author

How about adding something along the lines of dox --comments-only?

Might be an option for #156 as well, if I understand that problem correctly.

@tj
Copy link
Owner

tj commented Mar 25, 2015

js only IMO, could use esprima some day, that would be nice and reduce some wacky parsing issues

@Twipped
Copy link
Collaborator

Twipped commented Mar 25, 2015

I have to agree with @tj. If it works with css as an incidental, awesome, and I'm open to making small tweaks to make things easier, but I think officially supporting CSS is beyond the scope of the lib.

Also, as far as I know, apostrophes are not valid for css selectors. The only valid use of single or double quotes in selectors is for element attribute contents (which would always be paired with a matching end quote).

@Twipped Twipped closed this as completed Mar 25, 2015
@tomek-he-him
Copy link
Contributor Author

@ChiperSoft actually, apostrophes are valid along with a bunch of other unexpected characters – you can see the link I posted originally.

How about a --comments-only flag:

  • to boost parse time (especially if we're about to use esprima)
  • and to allow using dox in ES6 projects now before they are fully supported, later in ES7 projects, later still in ES8?

@Twipped
Copy link
Collaborator

Twipped commented Mar 25, 2015

The issue here isn't that Dox is unable to parse the code outside of the comments. The only time dox parses the code is when running regexes against the next line to identify comment context. If it doesn't match, it still parses the comment and the code just fine, there's just no context object.

The issue is that dox is seeing the opening of a string literal, which it should not parse a comment inside of, and is then continuing forward through the file until it finds another matching quote to close the string literal (or, until it hits the end of the file). There's no way to solve that for what is technically invalid JS code. If we ignore string literals then we start having loads of false positive matches for comments. Even if we interpreted a line ending as a premature termination of the string literal, that would still result in dox losing portions of the file.

In theory we could make it ignore quotes with a slash in front of them, but since that's not valid JS and could result in wrong behavior inside legitimate JS, I'm inclined against that.

@tomek-he-him
Copy link
Contributor Author

The only time dox parses the code is when running regexes against the next line to identify comment context.

Thanks, that makes things clear.

since that's not valid JS and could result in wrong behavior inside legitimate JS, I'm inclined against that

I totally agree.

Even if we interpreted a line ending as a premature termination of the string literal, that would still result in dox losing portions of the file.

Hmm, to me that wouldn't seem a bad idea after all. According to the spec a newline isn't a valid string character – except when directly preceeded by a \ – then it becomes a LineContinuation.

I guess such a fix would solve the issue temporarily. But if you plan to switch to esprima sooner or later, we'll need to look for another solution anyway.

@trusktr
Copy link
Contributor

trusktr commented Mar 26, 2015

In theory we could make it ignore quotes with a slash in front of them, but since that's not valid JS and could result in wrong behavior inside legitimate JS

You mean outside of a string literal \' is invalid, right? True, and in that case, yeah, we can't simply handle that for CSS because then that means we'd need to explicitly add support for CSS to dox which isn't dox's intended use case out of the box.

@trusktr
Copy link
Contributor

trusktr commented Mar 26, 2015

I could see the benefit of --comments-only. It'd basically just result in empty contexts, ignoring all code.

I'd could entertain the idea of dox supporting different languages by simply letting methods like parseCodeContext (and others) be overridden in a recommended way. A "plugin" would require dox internally, then override dox's methods (with no need for any actual "plugin architecture").

var dox = require('dox')
require('dox-css')

dox.parseComments(cssCode, options)

The word "dox" is generic enough too.

@trusktr
Copy link
Contributor

trusktr commented Mar 26, 2015

Dox would need to have a backup of all default functions, so then when plugins override these functions, it would be easy to do

var dox = require('dox')
require('dox-css').init()

dox.parseComments(cssCode, options)

dox.reset() // restore original functions. The public functions don't actually hold code, just wrap the original internals.
require('dox-ruby').init()

dox.parseComments(rubyCode, options)

Basically dox does the same thing internally as any "plugin" would do to override dox methods.

@tomek-he-him
Copy link
Contributor Author

I'd could entertain the idea of dox supporting different languages by simply letting methods like parseCodeContext (and others) be overridden in a recommended way. A "plugin" would require dox internally, then override dox's methods (with no need for any actual "plugin architecture").

This is a great idea @trusktr! I was already thinking of forking dox, or creating a minimal, stupid version of dox which only really parses tags. But this has a lot more sense!

The downside I see is that we'd need to make internal dox functions public. But maybe dox is already stable enough for such a move.

@trusktr
Copy link
Contributor

trusktr commented Mar 26, 2015

@tomekwi Well the internal functions wouldn't exactly be public. They'd be stored internally, privately, and proxied or adapted from the internal API to the public API, and so you'd only be able to override the public methods. Those methods like parseComments, parseCodeContext, trimIndentation, etc, are already all public.

We'd just need to decide what the public API should be, then adapt the public API to the internally stored things.

@trusktr
Copy link
Contributor

trusktr commented Mar 26, 2015

@ChiperSoft What are you thoughts on that? @tomekwi Similar discussion was happening here: #159

I'll just make a new issue specifically for the idea of making an overridable public API: #160

@Twipped
Copy link
Collaborator

Twipped commented Mar 26, 2015

@trusktr see my response on #160. I've got a plan for how I'm going to attack it. I'll start on it this weekend.

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

No branches or pull requests

4 participants