Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

Fixes avrodoc code blocks #110

Closed
wants to merge 1 commit into from
Closed

Fixes avrodoc code blocks #110

wants to merge 1 commit into from

Conversation

cassiedoll
Copy link
Member

This is the smallest possible change to fix the random code blocks that are appearing in our docs.

Essentially, if a newline has 4 spaces in front of it, avrodoc thinks thats a markdown code block.

Long term, I think we need to fix our style guide to indent avro records by 2 spaces only, and then shift the bodies of the /** */ comments over by 1 space, so that a paragraph only ever has 3 spaces in front of it. (or - someone can patch avrodoc with a better fix :)

@adamnovak
Copy link
Contributor

Do you mean "newline" i.e. space space space space \n, or do you mean a paragraph? Because it looks like your commit tries to get rid of second paragraphs that are indented 4 spaces.

Maybe the solution here is to remove empty lines from comments, so there are no second paragraphs? Would that work?

As it is, with this commit we now have comment blocks with two different indentation levels in the same block.

@cassiedoll
Copy link
Member Author

Yes - sorry about that. I meant, uh, newline x 2 - otherwise known as a paragraph :)

We could definitely remove empty lines from comments, but that's a little limiting for the longer explanations (of which I hope we have more of some day!) My long term suggestion was something like this:

record x {
../**
...paragraph #1
...
...paragraph #2
..*/
..string y;
}

So we never hit the 4 space limit with normal indentation.

If we can agree on something like that I can reformat all of the code in a different pr (and drop this one). My main goal was to quickly fix the docs on ga4gh.org, so at least @skeenan can patch this for now before we do something better.

@adamnovak
Copy link
Contributor

How does it handle something like:

record {
../**
....words words words

....words words
..*/
}

We could just be careful not to have indentation spaces on lines with no
content if it takes that.

An alternative style would be:

record {
../**
..words words
..
..words words
..*/
}

I.e. not indenting the comment relative to the delimiters.

I don't like the single-space indent, since a conditional mixture of 2- and
1-space tabs is not something I know how to configure in any editor.

Maybe the right long-term solution is to just fix avrodoc to intelligently
strip off the outer indentation of the comment contents before parsing the
markdown. The problem is that it's keying on the Markdown for code block,
which is just an indented paragraph, right?

Anyway, it seems like it will take a non-trivial amount of time to work out
what we actually want it to look like, so I guess +1 on this PR so we can
generate good docs today.

On Wed, Jul 30, 2014 at 11:06 AM, cassiedoll notifications@github.com
wrote:

Yes - sorry about that. I meant, uh, newline x 2 - otherwise known as a
paragraph :)

We could definitely remove empty lines from comments, but that's a little
limiting for the longer explanations (of which I hope we have more of some
day!) My long term suggestion was something like this:

record x {
../**
...paragraph #1
...
...paragraph #2
..*/
..string y;
}

So we never hit the 4 space limit with normal indentation.

If we can agree on something like that I can reformat all of the code in a
different pr (and drop this one). My main goal was to quickly fix the docs
on ga4gh.org, so at least @skeenan https://github.com/skeenan can patch
this for now before we do something better.


Reply to this email directly or view it on GitHub
#110 (comment).

@cassiedoll
Copy link
Member Author

I believe this is the relevant code in avrodoc.

If that logic spits out the "code_block" result, then the comment gets wrapped in pre/code tags.

(I think that says your first example would still trigger the code tag)

The real fix is definitely to patch avrodoc (because it should take into account the initial indent of the overall comment block in that loop). We could also fork a copy which never uses code_block style for a simple indent (I don't think we ever want that to happen) - that would be an easy hack, but we'd have to maintain our own version.

I'm open to whatever everyone else wants to do.

@delagoya
Copy link
Contributor

Actually avrodoc is actually "doing the right thing" with respect to markdown formatting.

This behaviour/bug is the fault of avro-tools.jar that is outputting the comment strings to JSON that avrodoc needs. Specifically avro-tools.jar is not stripping the leading white space for indented comments. It should really adjust the comment by the indentation level from the IDL file.

I'll create an example to illustrate the issue in a second. On a train with bad wifi.

@delagoya
Copy link
Contributor

Here are some examples for the underlying problem with Markdown formatting when translating from Avro IDL to Avro JSON to HTML

https://gist.github.com/delagoya/c2adc3660d2b2671616d

@cassiedoll
Copy link
Member Author

blah. so - do we hack avrodoc to work around the issue, try and fix avro-tools.jar, or change our comment style?

@adamnovak
Copy link
Contributor

I think the Right Way would be to change avro-tools.jar, which means making
changes against the official Apache Avro SVN.

The first step would probably be complaining on the Avro mailing list,
followed by an issue on their Jira.

On Wed, Jul 30, 2014 at 2:02 PM, cassiedoll notifications@github.com
wrote:

blah. so - do we hack avrodoc to work around the issue, try and fix
avro-tools.jar, or change our comment style?


Reply to this email directly or view it on GitHub
#110 (comment).

@fnothaft
Copy link
Contributor

From a pragmatic perspective, I think we should change our comment/indent style and open a JIRA issue on the Avro project in parallel.

@adamnovak
Copy link
Contributor

+1 on that. We can't just sit around waiting for them to take our random
patch.

On Wed, Jul 30, 2014 at 5:33 PM, Frank Austin Nothaft <
notifications@github.com> wrote:

From a pragmatic perspective, I think we should change our comment/indent
style and open a JIRA issue on the Avro project in parallel.


Reply to this email directly or view it on GitHub
#110 (comment).

@fnothaft fnothaft mentioned this pull request Jul 31, 2014
@fnothaft
Copy link
Contributor

I've opened #111; I'm +1 to @cassiedoll's change as it is sufficient, and will try to put together #111 in the next week (unless anyone else wants to do it).

@delagoya
Copy link
Contributor

We can just un-indent multi line comments. Will make the schema terrible to view, but it will fix the avrodoc issue. Single line comments will be ok as is.

On Jul 30, 2014, at 8:41 PM, Frank Austin Nothaft notifications@github.com wrote:

I've opened #111; I'm +1 to @cassiedoll's change as it is sufficient, and will try to put together #111 in the next week (unless anyone else wants to do it).


Reply to this email directly or view it on GitHub.

@cassiedoll
Copy link
Member Author

Note: for some reason this fix did not work for the Stephen's docs on ga4gh.org, so he ended up removing the paragraph breaks.

So let's definitely not merge this in. Question - should we use Adam's suggestion from before and just never use multi-paragraph comments? (and so just leave style as is)

@cassiedoll
Copy link
Member Author

Closing this in favor of #111

@cassiedoll cassiedoll closed this Aug 1, 2014
@cassiedoll cassiedoll deleted the doc-bug-fix branch August 1, 2014 18:38
dcolligan pushed a commit to dcolligan/ga4gh-schemas that referenced this pull request Jul 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants