METRON-660 [Umbrella] up-to-date versioned documentation #429
Conversation
This is a fantastic improvement to what we have, and even though I'm just starting to dig in, this looks really good. I'll add any comments I have as I go through it, but this is great. |
site-book/src/site/site.xml
Outdated
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
--> | ||
<project name="Falcon" xmlns="http://maven.apache.org/DECORATION/1.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change the name from 'Falcon'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Missed that one. Thanks for pointing it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Bullet points get a little weird in some places. (e.g. in the metron-maas-service README). Looks like, if we don't have a newline before the first bullet, it looks like it just includes it as part of the previous sentence. I found a few in the metron-maas-service README, but it wouldn't surprise me if more show up. Same actually applies to code blocks. |
Awesome stuff, I'm taking a look now. Related to @justinleet's comment, it looks like there's an issue with nesting ordered and unordered list. An example is on the main Also, regarding the bullet point issue, there is an example on the main page under "Navigating the Architecture", under metron-parsers, and I'm sure in various other locations. |
@justinleet and @JonZeolla , the issues you are describing with bullets and codeblocks sound like things I worked hard to fix, and they work on my system. Please see new screenshot at https://issues.apache.org/jira/secure/attachment/12850028/METRON-660-screenshot2.png I'm running the re-writer under Mac OS X with GNU Awk version 4.1.1. Would you both please tell me what system you're running and what |
Mac 10.12.2, awk version 20070501 |
@JonZeolla , interesting. That would be equivalent to gawk version 3.1.5 or 3.1.6. I see now that my updated awk is from a homebrew installation. Would you be willing to update awk? If you already use homebrew, all you have to say is |
@mattf-horton Homebrew gawk seems to work out well for me. Not sure what the implementation difference is between the two though. |
Guys, to make it easier to distinguish between tool problems vs content bugs, I've uploaded a tarball of the full site-book as built on my platform, at site-book_0.3.0_20170130.tar.gz Could you please compare problems you are seeing in your build of the book, vs what you see in my build? Thanks. BTW, I'm not minimizing the tool problems, they need to be resolved. But we do need to know whether we are looking at a tool problem or a content bug! :-) |
@mattf-horton gawk man page: Obvious awk issue in output (that doesn't show up with gawk), that I should have noted, but apparently just shut my brain off for:
|
@justinleet , yah, that's typical of what I called "paragraph munching". The triple-backtick delimiter is being consumed as text, then interpreted as an empty double-backtick, followed by single backtick that starts a code-phrase, hence the ugly highlighting. Was this with the homebrew awk? And can you confirm that
points at ../Cellar/gawk/4.1.1/bin/awk ? It's most likely an awk problem, because that's where the re-writing gets done.
|
@justinleet , hi, sorry our responses crossed. Looks like the old awk is rejecting the regex, probably the On my Centos7 test VM, awk --version gives 4.0.2. Probably will work, but I'll try it out. |
@mattf-horton I think we're dealing with different implementations of awk, rather than just a pure versioning issue. I'm guessing it works on that VM, because that version number looks like a Gawk number. I'm pretty sure it's not working on the default Mac, because it's a different implementation entirely. At that point, we're saying that whoever is building has to install gawk if they're on Mac (which a good portion of people are). Could the awk script can be relatively easily refactored to not use gawk specifics functions/features? I simply don't have enough awk knowledge to know how easy/hard that would be |
@justinleet , No, the advanced awk features are heavily used. Well, I guess this is a good reason to convert to a python script after all. I'll give it whack. |
site-book/bin/generate-md.sh
Outdated
if [[ "$OSTYPE" =~ "darwin" ]] ; then | ||
# Macs need an argument after -i option | ||
sed -i '' -e "${HREF_REWRITE_LIST[ $(( i + 1 )) ]}" "${HREF_REWRITE_LIST[$i]}" | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest handling this slightly more explicitly. My two thoughts are:
- Approach it similar to here, using a
case "${OSTYPE}"
to watchdarwin*
andlinux*
, and then a*)
that alerts the user. - By adding an elif for either
"${OSTYPE}" == linux*
or"${OSTYPE}" =~ "linux"
, and then anelse
that alerts the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the path of least resistance here just to provide a docker container with the right version of awk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JonZeolla thanks for the example pointer. Fixed.
@james-sirota , yes maybe sorta. But that's work too, and the on-going nuisance for people trying to use it isn't adequately balanced for the effort. I'm almost done with the conversion to python.
Ok, I'm going to hold off on any additional review until the move to a python script. |
Okay, @JonZeolla , @justinleet , and others, the awk script has been replaced by a python script. The results of the dialect fix-up are identical to the awk script's results across all 37 .md files. Thanks. |
BTW, the instructions for use are unchanged, since the fix-up script is invoked from within the bin/generate-md.sh script. |
@mattf-horton, thanks for taking the time/effort to migrate the script. Tried it out and it works great. The only thing I'd like to see is the indentation in the Python script made consistent. I think spaces is more standard Python and swapping potentially creates issues because of the way Python handles tabs (searching around online suggests it's treated as 8 spaces). |
@justinleet thanks for the catch. Fixed. |
+1, this is great. |
Great job with the migration to python, things look much much better now. I did notice that the |
Hi @JonZeolla , you have good eyes! I missed those cases, which are only visible where H1 headers have been used in the body of the document, AND there are TOC or other internal links to those headings. Eeesh, that's an unfortunate deficiency in the doxia-markdown plugin. Apparently it does not generate named anchors for the H1 headers, perhaps under the assumption there will only be one at the beginning of the doc? At any rate, I've opened two new jiras:
But I request that we allow these to be addressed as follow-on tasks, so we can get the site-book as it is into the public's hands. Thanks. |
@JonZeolla you pushed my perfectionist button :-) This latest push fixes the problem with anchors on H1 headers. The generated html is unchanged except for the insertion of anchor lines next to H1 header lines. The visuals appear the same. |
@cestella , as Release Manager you can choose whether to accept this last commit (if @JonZeolla has time to review it) or just roll in the previous change set. I'm ok either way. |
+1 looks good to me @mattf-horton. Reviewed the latest commit and ran it up/did some brief validation. |
I'm +1 as well, merge master in @mattf-horton and I'll commit. Great work; I've been lurking on this one and like what I saw. 🥇 |
…rite script to conform Github-MD source files to doxia-markdown. Also misc fixes to markdown files.
760908d
to
7ac36e2
Compare
Thank you all for your efforts reviewing this. @cestella , I have rebased to master and re-validated. The only conflict was a trivial edit in one doc file. Per discussions in the mailing lists, I also squashed most of my commits to a single one, leaving the necessary to preserve the attribution of @mmiklavc 's assist. Hence a total of three commits. |
This has come out rather well, I think. The integration is still open to discussion. Currently it is in a stand-alone, versioned-with-the-code sub-directory and sub-project. The idea is that a release manager would build the site-book (following the instructions below), then copy it into a versioned subdirectory of the (unversioned) public site, to publish it along with each code release.
To build the book, do the following:
In any git clone of incubator-metron containing the site-book subdirectory,
It only takes a few seconds. You may now view your copy of the book in a browser by opening
file:///your/path/to/incubator-metron/site-book/target/site/index.html
. On a Mac, you can just sayopen target/site/index.html
Enjoy! For code review purposes, the key files under site-book/ are:
No .md files were harmed in the making of this PR! The goal was "If it works in Github-MD, don't edit the source .md file -- fix it in the re-writer."
The only thing hardwired is the handling of the few image files used; that can be removed when it becomes important. Some may object to the awk script; if you wish to translate it into python, I'll be more than happy to accept the PR :-)
Thanks,
--Matt