Adds flag statement (TODO, FIXME, ...) parsing for docs #3519

samueleaton · 2016-11-07T22:30:27Z

Adds parsing for flag statements (TODO, FIXME, BUG, ...) for docs as discussed in issue #3460

I had to create a workaround because TODO: and other keywords are not part of the markdown parser.

The code I added:
(1) Makes sure that lines starting with TODO: are isolated into their own paragraph
(2) Splits the HTML output by paragraph, and then replaces TODO: with the relevant HTML

If there is another way to do this (e.g. with the markdown parser) point me in the right direction and I'll see what I can do.

Screenshot

oprypin · 2016-11-07T22:36:00Z

~~What is the point of this?~~

Sija · 2016-11-07T22:47:24Z

@BlaXpirit @samueleaton explained his reasoning behind this PR in #3460.
@samueleaton I'd add support for FIXME too.

ysbaddaden · 2016-11-08T00:50:21Z

As well as NOTE and OPTIMIZE.

samueleaton · 2016-11-08T01:07:55Z

Should TODO, NOTE, FIXME, and OPTIMIZE be different colors or all orange?

Different Colors:

All Orange:

EDIT: I'm on a little macbook air with a crappy screen. I'll make sure the colors are good when I'm on my good monitor

keplersj · 2016-11-08T01:10:29Z

I like the different color! Perhaps also add support for DEPRECATED?

samueleaton · 2016-11-08T01:23:13Z

@keplersj If we were to do DEPRECATED, would it be the same format with a colon? Would the text that follows the colon be an explanation on why it is deprecated?

Like this:

# one of our beloved customers
#
# TODO: create methods for this class
#
class Customer

  # fetches a customer from the database by id
  #
  # DEPRECATED: Use *retrieve* instead
  # TODO: query db for customer  
  # TODO: return nil if customer not found with id  
  # TODO: return an instance of `Customer` if customer exists  
  def self.fetch(customer_id : String)
  end
end

I personally think deprecated should be on the same line as the method description:

# one of our beloved customers
#
# TODO: create methods for this class
#
class Customer

  # fetches a customer from the database by id (DEPRECATED)
  #
  # TODO: query db for customer  
  # TODO: return nil if customer not found with id  
  # TODO: return an instance of `Customer` if customer exists  
  def self.fetch(customer_id : String)
  end
end

This way the deprecated flag would be next to the method definition rather than underneath it. It would also make it so you can't put multiple DEPRECATED flags, which seems appropriate.

keplersj · 2016-11-08T02:49:53Z

@samueleaton I like the first one better, I think it's a lot like Scala's deprecated annotation.

samueleaton · 2016-11-08T05:59:52Z

As of the latest commit, the following will be parsed in the docs:

Red flags: FIXME, BUG, DEPRECATED
Orange flags: TODO
Yellow flags: NOTE
Green flags: OPTIMIZE

samueleaton · 2016-11-08T06:12:52Z

BUG and FIXME are seem similar. Is there one that people are more fond of? I am in favor of BUG. Its shorter and is a more common word (easier to remember).

ysbaddaden · 2016-11-08T07:10:54Z

FIXME is more common. It may no be a bug, something poorly written for example.

RX14 · 2016-11-08T11:39:53Z

src/compiler/crystal/tools/doc/generator.cr

@@ -241,6 +251,17 @@ class Crystal::Doc::Generator
    end
  end

+  # adds extra line break to TODO lines
+  def isolate_flag_lines(string)
+    string.split(/\n/).map do |line|


I feel like the regex here and the regex on line 235-240 could be replaced by a method which takes a line and returns {flag, text} or nil. We can even combine them, as HTML passes through markdown unimpeded.

For example:

if flag, text = parse_flag(line) io << %(#{flag} #{text}\n) else io << line end

which requires only one pass.

Are you saying to remove the regex completely or just to consolidate the regex into a single method?

@RX14 I remember why I had to do the regex twice (let me know if your solution would still apply here).

If I were to replace the TODO: with the HTML before the line is parsed, then the markdown parser will escape the HTML. But if I replace the TODO: with HTML after the parser, then each TODO: statement is not guaranteed to be in its own paragraph.

That is why:

before the markdown parser I am adding an extra \n after each TODO: line (to guarantee that each line is in its own paragraph)

after the markdown parser I am replacing the TODO: with the HTML (because the HTML would be escaped otherwise)

I was assuming that html passed through the markdown parser unescaped, because that's what I've seen most engines do. Github lets you use certain html tags in markdown.

I think that it is probably a bad default to let html through, but it should at least be an option. However if we went this route we would have to sanitize html to include only certain tags (github has a list they let through their markdown). It might be worth looking at nature markdown implementations to see how this is done.

However my first suggestion with the each_line and join with block are still valid with this workaround.

samueleaton · 2016-11-08T14:32:17Z

@ysbaddaden Two Questions: (1) Do we want to require that every flag ends in a colon? (2) Do we want all flags to be required to be uppercase?

Ending in Colon

For example, here the DEPRECATED would become a flag:

class Customer

  # fetches a customer from the database by id
  #
  # DEPRECATED: This method was buggy and is deprecated 
  def self.fetch(customer_id : String)
  end
end

But here the DEPRECATED would not become a flag:

class Customer

  # fetches a customer from the database by id
  #
  # DEPRECATED
  def self.fetch(customer_id : String)
  end
end

Require Uppercase

Do we want this to work?

class Customer

  # fetches a customer from the database by id
  #
  # deprecated: This method was buggy and is deprecated 
  def self.fetch(customer_id : String)
  end
end

Sija

Both FIXME and BUG should be supported, there's no need to chose one over the other.

samueleaton · 2016-11-08T14:55:20Z

@Sija Gotcha. I reverted the commit.

ysbaddaden · 2016-11-08T14:58:14Z

No big rule, but I would say that the colon is optional, as long as the definition is on its own line, whereas uppercase is mandatory.

That is:

TODO
TODO incredible feature
TODO: support ARM

samueleaton · 2016-11-08T21:25:51Z

I updated the code to also flag the keywords in the main project README.

Before

After

Sija · 2016-11-08T21:48:19Z

src/compiler/crystal/tools/doc/generator.cr

+    end.join("<p>")
+  end
+
+  # Adds extra line break to flag keyword lines
  def isolate_flag_lines(string)
    string.split(/\n/).map do |line|
      if line =~ /^ ?(TODO|FIXME|BUG|NOTE|OPTIMIZE|DEPRECATED):?/


I'd use a hash containing flags and corresponding colors — or even just an array with flags, moving the colors into the css itself.

@Sija Something like this?

FLAG_COLORS = { "BUG" => "red", "DEPRECATED" => "red", "FIXME" => "red", "NOTE" => "yellow", "OPTIMIZE" => "green", "TODO" => "orange" } FLAGS = FLAG_COLORS.keys def generate_flags(string) FLAGS.reduce(string) do |str, flag| flag_regexp = /\s*#{flag}(:| )?/ element_sub = %(#{flag} ) str.gsub(flag_regexp, element_sub) end end

yep, and you could reuse it in isolate_flag_lines method too.

samueleaton · 2016-11-09T15:08:16Z

Here is the latest on colors:

luislavena · 2016-11-10T13:53:54Z

@samueleaton seems that the failure in Travis is caused by formatting issues. Can you reformat tools/doc/generator.cr using the compiler from the checkout (not the 0.19.4 release) and push again?

Thank you.

Sija · 2016-11-10T14:23:27Z

src/compiler/crystal/tools/doc/generator.cr

@@ -226,9 +237,11 @@ class Crystal::Doc::Generator
  end

  def doc(context, string)
-    String.build do |io|
-      Markdown.parse string, MarkdownDocRenderer.new(context, io)
+    str = isolate_flag_lines string


why create new variable? couldn't that just be string = isolate_flag_lines string?

Been doing some functional programming and it is showing 😅 . I can change it.

I could probably just remove that line altogether and do this

Markdown.parse isolate_flag_lines(string), MarkdownDocRenderer.new(context, io)

Keeping it in a separate line seems clearer to me.

Sija · 2016-11-10T14:28:28Z

src/compiler/crystal/tools/doc/html/css/style.css

+  padding: 2px 4px 1px;
+  border-radius: 3px;
+  margin-right: 3px;
+  font-size: 11px;


I'd suggest folding multiple border declarations into:

border: 1px solid transparent;

and l8r in span.flag.<color>:

border-color: #color;

This came to mind but I i didn't think it was necessary. In hindsight it is better that way. Good eye. 👁 👍

RX14 · 2016-11-10T17:32:56Z

src/compiler/crystal/tools/doc/generator.cr

@@ -52,7 +63,7 @@ class Crystal::Doc::Generator
      Markdown.parse body, MarkdownDocRenderer.new(program_type, io)
    end

-    File.write "#{@dir}/index.html", MainTemplate.new(body, types, repository_name)
+    File.write "#{@dir}/index.html", MainTemplate.new(generate_flags(body), types, repository_name)


I actually thing that instead of duplicating changes in this method, this can be replaced with a call to doc(program_type, body), where body is the old definition.

This hasn't been fixed.

Fixed it. I even made an optimization where it won't even run the markdown parser on an empty string for the README if the file doesn't exist.

Sija · 2016-11-12T15:54:42Z

GTG?

luislavena · 2016-11-12T16:00:39Z

I think is good enough to be merged, @samueleaton can you rebase and squash all the changes?

Thank you.

asterite · 2016-11-12T16:21:35Z

O still don't quite understand why you wouldn't put a big TODO in public documentation. Is it to encourage pull requests? Is it a remainder for self? Is it to make a library look incomplete?

I'm not against this change, I just can't see the usefulness.

luislavena · 2016-11-12T16:25:59Z

Hello @asterite I'm not sure to follow:

why you wouldn't put a big TODO in public documentation

You mean the opposite?

Since docs of your project (public or private) will be processed by crystal doc, I personally find it helpful to highlight any of these notes in our methods or classes.

That of course doesn't cancel or render pointless to have a TODO section on the README file or even a ROADMAP.md document.

I don't think they are mutually exclusive.

samueleaton · 2016-11-17T01:49:49Z

@asterite its squashed and the reviews are fixed. You will also need to merge the pending request at crystal-book #45.

RX14 · 2016-11-17T09:29:20Z

src/compiler/crystal/tools/doc/generator.cr

-    body = String.build do |io|
-      Markdown.parse body, MarkdownDocRenderer.new(program_type, io)
-    end
+    body = if filename


I much prefer the old style of having body = inside the block. Also isn't this String.build redundant? Doesn't doc return a string anyway?

fixed. although I would have done...

body = filename ? doc(program_type, File.read(filename)) : ""

... but i don't think it would have been received well

Sija · 2016-11-19T21:37:02Z

@asterite

asterite · 2016-11-19T21:49:01Z

Is this ready to merge? If so, I'll merge it

Sija · 2016-11-19T21:50:53Z

Seems like it :)

RX14 · 2016-11-19T21:50:59Z

I can't find anything to complain about any more, at least :)

Maybe if we hit performance issues in the doc generator the regex can be replaced but I think it's a non-issue for now.

asterite · 2016-11-20T00:22:59Z

I just tried it and the resulting doc is broken. There are double new lines everywhere. See screenshot:

So for now I won't merge this.

samueleaton · 2016-11-20T03:42:35Z

@asterite I found and fixed the problem. I expected string.each_line.join("", io) do |line, io| to remove the \n at the end of each line in the block. (as would happen with string.split("\n").each do |line|).

asterite · 2016-11-20T15:02:24Z

Looks good now, thank you! ❤️

Sija · 2016-12-02T04:24:40Z

Multiline flag statements ...

# NOTE: This some rather long long long long long
# note split to more lines.

... end up like this (which probably is a bug):

RX14 · 2016-12-02T10:20:53Z

@Sija I think that that is correct. Your example looks like 2 lines to me. However there should be a way to have multi line todos, maybe indent?

Sija · 2016-12-02T12:58:45Z

@RX14 I think that particular paragraphs and code blocks are supposed to be surrounded by an empty line, which should allow you for writing multiline statements without additional indentation.

# Multiline comments have all lines
# concatenated until empty line is found.
#
# This would be next paragraph.

RX14 · 2016-12-02T13:40:05Z

So what's the difference between these?

# TODO this is a long string which is a
# TODO which wraps into another line.

# TODO this is todo 1
# TODO this is todo 2

I meant to first one to render "this is a long string which is a TODO which wraps into another line." and the second to render 2 TODOs.

samueleaton · 2016-12-02T19:35:51Z

@Sija That is how it was intended. But I agree with @RX14 that there should be support for multiline flags.

However, I would think that users are more likely to have 2 or more flags in a row than have a really long flag, so I don't think a set of flags should have an extra blank line between each flag.

Sija · 2016-12-02T20:14:51Z

I agree that TODO flags are rather short, but NOTE ones easily could span far more than 80 characters which is often used as a soft wrap limit. I also agree that every flag separated by an extra
newline would be lengthy. Indentation seems like a good way to handle that.

…#3519)

samueleaton · 2016-12-16T02:43:25Z

@Sija I'm working on a project and now agree with you about the NOTEs extending more than a single line. 😭

So I think I'll make another PR with the following change: ~~Only add an extra line break to a flag line if the next line is also a flag line.~~ Just move the line break to the front of a flag line. simple.

Example

# My Class
#
# NOTE: Xxxxxx xxxxx xx xxxxxxx x xx.
# NOTE: Yyyyyy y yyy y y yyy yyy yy yyyyy yyyy
# yyy yyyyy y yyyy y yyyyy yyyyyyy yy yy yyyy yy
# yyyyyyyyy yyyyyyyy y yy yy yyyy y.
# NOTE: Zzzzz zzzzzz zzz zzzzzz.
#
# Qqqq qq q q q qqqqq qqq q qqqq qq q qqq q qqqq q qqqq q.
class MyClass
end

RX14 reviewed Nov 8, 2016

View reviewed changes

samueleaton changed the title ~~Adds TODO statement parsing for docs~~ Adds flag statement (TODO, FIXME, ...) parsing for docs Nov 8, 2016

Sija reviewed Nov 8, 2016

View reviewed changes

samueleaton force-pushed the doc-todo branch from 9fd9bed to a5e4d76 Compare November 8, 2016 14:54

Sija reviewed Nov 8, 2016

View reviewed changes

Sija reviewed Nov 10, 2016

View reviewed changes

samueleaton mentioned this pull request Nov 10, 2016

Add documentation for doc flags crystal-lang/crystal-book#45

Merged

RX14 reviewed Nov 10, 2016

View reviewed changes

samueleaton force-pushed the doc-todo branch from 306691b to e4efc0b Compare November 12, 2016 23:35

samueleaton force-pushed the doc-todo branch from e4efc0b to 76ac1c1 Compare November 17, 2016 01:44

RX14 requested changes Nov 17, 2016

View reviewed changes

Adds flag parsing to docs

503f561

samueleaton force-pushed the doc-todo branch from 76ac1c1 to 503f561 Compare November 17, 2016 14:11

Remove extra linebreak

32a8557

asterite merged commit 1928ce5 into crystal-lang:master Nov 20, 2016

asterite added kind:docs kind:feature labels Nov 20, 2016

asterite added this to the 0.20.0 milestone Nov 20, 2016

Sija referenced this pull request Nov 22, 2016

Another missing changelog entry

6eee1e9

firejox pushed a commit to firejox/crystal that referenced this pull request Dec 12, 2016

Adds flag statement (TODO, FIXME, ...) parsing for docs (crystal-lang…

967544f

…#3519)

samueleaton mentioned this pull request Dec 18, 2016

Move line break to front of line break rather than end #3717

Merged

samueleaton mentioned this pull request Feb 1, 2017

Support of other flag types in addition to "deprecated" apidoc/apidoc#592

Open

This was referenced Jun 27, 2017

Improve Markdown implementation - Or: Should we separate compiler tools? #4613

Closed

Command line tool to list doc comment flags #4629

Closed

Adds flag statement (TODO, FIXME, ...) parsing for docs #3519

Adds flag statement (TODO, FIXME, ...) parsing for docs #3519

Conversation

samueleaton commented Nov 7, 2016 • edited Loading

oprypin commented Nov 7, 2016 • edited Loading

Sija commented Nov 7, 2016 • edited Loading

ysbaddaden commented Nov 8, 2016

samueleaton commented Nov 8, 2016 • edited Loading

keplersj commented Nov 8, 2016

samueleaton commented Nov 8, 2016

keplersj commented Nov 8, 2016

samueleaton commented Nov 8, 2016 • edited Loading

samueleaton commented Nov 8, 2016

ysbaddaden commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Nov 8, 2016 • edited Loading

Ending in Colon

Require Uppercase

Sija left a comment

Choose a reason for hiding this comment

samueleaton commented Nov 8, 2016

ysbaddaden commented Nov 8, 2016

samueleaton commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueleaton commented Nov 9, 2016

luislavena commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented Nov 12, 2016

luislavena commented Nov 12, 2016

asterite commented Nov 12, 2016

luislavena commented Nov 12, 2016

samueleaton commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented Nov 19, 2016

asterite commented Nov 19, 2016

Sija commented Nov 19, 2016

RX14 commented Nov 19, 2016

asterite commented Nov 20, 2016

samueleaton commented Nov 20, 2016

asterite commented Nov 20, 2016

Sija commented Dec 2, 2016 • edited Loading

RX14 commented Dec 2, 2016

Sija commented Dec 2, 2016

RX14 commented Dec 2, 2016

samueleaton commented Dec 2, 2016 • edited Loading

Sija commented Dec 2, 2016

samueleaton commented Dec 16, 2016 • edited Loading

samueleaton commented Nov 7, 2016 •

edited

Loading

oprypin commented Nov 7, 2016 •

edited

Loading

Sija commented Nov 7, 2016 •

edited

Loading

samueleaton commented Nov 8, 2016 •

edited

Loading

samueleaton commented Nov 8, 2016 •

edited

Loading

samueleaton commented Nov 8, 2016 •

edited

Loading

RX14 Nov 10, 2016 •

edited

Loading

Sija commented Dec 2, 2016 •

edited

Loading

samueleaton commented Dec 2, 2016 •

edited

Loading

samueleaton commented Dec 16, 2016 •

edited

Loading