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

TINKERPOP-1447 Add some JavaScript intelligence to the documentation so that comments and output are not copied in a copy paste #766

Merged
merged 1 commit into from
May 12, 2018

Conversation

dkuppitz
Copy link
Contributor

https://issues.apache.org/jira/browse/TINKERPOP-1447

Added automatic tab generation for all evaluated code block in all docs. A random screenshot from the generated reference docs (for those who are too lazy to build the docs):

image

VOTE: +1

@@ -139,7 +140,7 @@ if [ ! ${SKIP} ] && [ $(grep -c '^\[gremlin' ${input}) -gt 0 ]; then
${lb} awk -f ${AWK_SCRIPTS}/language-variants.awk > ${output}

ps=(${PIPESTATUS[@]})
for i in {0..6}; do
for i in {0..7}; do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not able to follow why this is a loop from 0 to 7... can we include a comment or move 7 to a constant with a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, it should actually be 0 to 9, as it checks to exit code for each program in the pipe chain. Not sure when that went out of sync, but I'll fix it.

@robertdale
Copy link
Member

What about for those of us unable to build it?

sh docker/build.sh -d

 * source:   /usr/src/tinkermem/docs/src/recipes/olap-spark-yarn.asciidoc
   target:   /usr/src/tinkermem/target/postprocess-asciidoc/recipes/olap-spark-yarn.asciidoc
   progress: [=======================================================>                                            ] 55%java.io.FileNotFoundException: File file:/tmp/spark-gremlin.zip does not exist
Type ':help' or ':h' for help.
Display stack trace? [yN]pb(104); '----'


Last 10 lines of /usr/src/tinkermem/target/postprocess-asciidoc/recipes/olap-spark-yarn.asciidoc:

	at org.apache.spark.SparkContext$.getOrCreate(SparkContext.scala:2281)
	at org.apache.spark.SparkContext.getOrCreate(SparkContext.scala)
	at org.apache.tinkerpop.gremlin.spark.structure.Spark.create(Spark.java:52)
	at org.apache.tinkerpop.gremlin.spark.structure.Spark.create(Spark.java:60)
	at org.apache.tinkerpop.gremlin.spark.process.computer.SparkGraphComputer.lambda$submitWithExecutor$0(SparkGraphComputer.java:193)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
gremlin> 

xargs: /usr/src/tinkermem/docs/preprocessor/preprocess-file.sh: exited with status 255; aborting

@dkuppitz
Copy link
Contributor Author

Hmm, this was another ticket and should be solved in this branch as I rebased it after the latest release.

@robertdale
Copy link
Member

@dkuppitz I did see that. There was a dep added to the image. I have that rebuilding now. Will let you know the results.

@robertdale
Copy link
Member

Just what I was afraid of... the tabs look just like they do in the screenshots. And I don't mean that in a good way 😄

Seriously, this is fantastic. Multi-line works too.

My issues:

  • I prefer the nice, thin border that was previously used
  • I'd like to see lighter-weight, less glaring tabs - See also https://thrift.apache.org/ (Example)
  • actually reading some of the finer details, the WARNING under Mutating the Graph states that 'JAVA' or 'GROOVY' should appear in the code blocks. This doesn't work for me in this PR or 3.2.7 docs. Looks like something introduced in 3.2.6 causes the screen to be super extra wide.
  • Under SparkGraphComputer -> Loading with BulkLoaderVertexProgram there are multiple tabs that don't seem to work correctly.

It's also interesting in that we can now see what's rendered via console vs. hardcoded. Might need to revisit some of these sections and test for correctness.

BTW, rebuilding the base image did the trick. Alternatively, it looks like I could have just installed zip.

@dkuppitz
Copy link
Contributor Author

dkuppitz commented Dec 28, 2017

@robertdale I like the look :) Do you want to give it a try and tweak the CSS? I just took it from here, changed some colors and that's it. Maybe I just like the current look, because it just worked and I didn't have to mess too much with the CSS stuff.

actually reading some of the finer details, the WARNING under Mutating the Graph states that 'JAVA' or 'GROOVY' should appear in the code blocks. This doesn't work for me in this PR or 3.2.7 docs.

Not related to this PR, but you're right. That seems to be related to an AsciiDoc update as the blocks no longer react on mouse events. I guess we should just change the WARNING text accordingly.

@robertdale
Copy link
Member

@dkuppitz my css skills are close to nil. what little i did play with it, i'm not sure it looks any better. and in the course of doing so, I've grown accustomed to it.

In any case, one outstanding issue it the Loading with BulkLoaderVertexProgram tabs. The docker image uses awk 4.0.1. The result is that the tab sequence is 4,1,2,3. I'm not entirely sure what the cause is. It works correctly in awk 4.1.

@robertdale
Copy link
Member

@dkuppitz I pushed a CTR fix for invisibility to tp32/master. This fixes the screen from being rendered to 9999 pixels wide. It didn't help the code block language popup though.

@robertdale
Copy link
Member

In the 'Console' tab, muli-line statements work. They have the \ continuation after the callout. However, on the 'language' tab, these seem to be lost thus copy-paste multi-line statements do not work.

@robertdale
Copy link
Member

Excellent!
VOTE +1

@spmallette
Copy link
Contributor

I saw some problems using docker above to generate docs....is that still an issue? I'm getting this:

 * source:   /usr/src/tinkerpop/docs/src/reference/gremlin-variants.asciidoc
   target:   /usr/src/tinkerpop/target/postprocess-asciidoc/reference/gremlin-variants.asciidoc
   progress: [==============================================>                                                     ] 46%ImportError: No module named six in <script> at line number 1
Type ':help' or ':h' for help.
Display stack trace? [yN]jython.eval('from gremlin_python.process.traversal import *')
groovysh_parse: 2: expecting EOF, found ')' @ line 2, column 21.
   GraphSONVersion.V2_0).create().createMapper()
                       ^

1 error
Type ':help' or ':h' for help.
Display stack trace? [yN]bytecode = mapper.readValue(jython.eval('GraphSONWriter().writeObject(j)').toString(), Bytecode.class)
groovysh_parse: 2: expecting EOF, found ')' @ line 2, column 4.
   s()) f.deleteDir()
      ^

1 error
Type ':help' or ':h' for help.
Display stack trace? [yN]f = new File('/tmp/tinkergraph.kryo')
groovysh_parse: 2: unexpected char: ''' @ line 2, column 42.
   of a lambda in Gremlin-Python.'
                                 ^

1 error
Type ':help' or ':h' for help.
Display stack trace? [yN]pb(277); '<2> The default lambda language is currently Gremlin-Python.'
groovysh_parse: 2: unexpected token: , @ line 2, column 1.
   , the strategy is'
   ^

1 error
Type ':help' or ':h' for help.
Display stack trace? [yN]pb(411); 'encoded in the Gremlin.Net bytecode and transmitted to the Gremlin traversal machine for re-construction machine-side.'


Last 10 lines of /usr/src/tinkerpop/target/postprocess-asciidoc/reference/gremlin-variants.asciidoc:

* `Traversal.toList()`
* `Traversal.toSet()`
* `Traversal.iterate()`

=== Gremlin-Python Sugar

Python supports meta-programming and operator overloading. There are three uses of these techniques in Gremlin-Python that
makes traversals a bit more concise.

[source,python]

xargs: /usr/src/tinkerpop/docs/preprocessor/preprocess-file.sh: exited with status 255; aborting

I get the same error before and after deleting docker images.

@robertdale
Copy link
Member

Try rebuilding the docker images using master.

@robertdale
Copy link
Member

Hmm.. there doesn't seem to be much difference. Are you deleting both the hadoop and base images?

@spmallette
Copy link
Contributor

i deleted both when i did it, but i didn't rebuild from master - i built from this branch i guess

@dkuppitz
Copy link
Contributor Author

dkuppitz commented Jan 5, 2018

I rebuilt my containers using this branch and docker/build.sh -d just finished gremlin-variants.asciidoc.

@spmallette
Copy link
Contributor

tried again using images from this branch and died the same way. so weird when stuff like this happens with docker.

@robertdale
Copy link
Member

@spmallette did you save your console log?

@spmallette
Copy link
Contributor

don't have any of that stuff from my last attempt. what are you looking for exactly? i could fire it up again....

@robertdale
Copy link
Member

@spmallette delete all of your images and then rebuild everything with docker/build.sh -d |tee /tmp/console.log and send me that output file. I wonder if it's choking on some python download. I've had that problem before. I just rebuilt everything from scratch and it still works. (I think I use master to build the image because I can override the maven url in .travis.install-maven.sh)

@spmallette
Copy link
Contributor

I'm posting this to all open PRs of current relevance - this PR needs to be rebased against the branch it is targeted against given a broken python dependency that was published to pypi a day or so ago. I've pushed a fix on tp32 and master at this point and David Brown is working on getting an issue raised with the project that initiated the problem.

@spmallette
Copy link
Contributor

wow - thanks to @robertdale with 34f924e because i can finally generate these docs with docker. i guess the question now is whether we rush to immediately look to merge this or push off until next release. i kinda feel like we should do the latter and not be hasty. I'd like some time to put in some other code in certain places and play around with it a bit before we get this on to a release branch and i don't know that i will have time for such things between now and first week of april when 3.2.8/3.3.2 are going out the door. any thoughts to the contrary?

@dkuppitz
Copy link
Contributor Author

I have no problem pushing it off. It's unlikely that any other PR is going to affect the changes in this PR, so rebasing should be a no-brainer at any time.

@spmallette
Copy link
Contributor

I was hoping to see us merge this for next release, but now that we have to release this patch because of the groovy bug, there won't be a lot of time to do much with the cool changes you have here. I was hoping we'd be able to see at least some GLV code translation in place for an actual release. Maybe that doesn't matter, in which case I guess we can merge this. Your call in that respect.

A question though about the tab system...the tabs look good with two options as shown in your screenshot, but we have Console, Groovy, Java (maybe we wouldn't do that), Python, Javascript and .NET - how does it look when we have all those? Does the tab system scale that way?

@dkuppitz
Copy link
Contributor Author

It does scale to up to 4 tabs. Should we ever have more, we only need to tweak the maxTabs property and regenerate the CSS.

@dkuppitz dkuppitz changed the base branch from tp32 to tp33 April 26, 2018 00:03
@spmallette
Copy link
Contributor

I just generated docs locally on this branch and i didn't get the tabs - it showed radio buttons. something wrong with the stylesheet after rebase maybe?

@dkuppitz
Copy link
Contributor Author

image

Worked well for me.

@spmallette
Copy link
Contributor

i guess i will try again.............................................................................................................................

@spmallette
Copy link
Contributor

Ok - the issue is related to how the tabs.css is in there:

<link rel="stylesheet" href="/docs/3.2.8-SNAPSHOT/stylesheets/tabs.css" />

if that were a relative url like everything else, we'd have it.

@spmallette
Copy link
Contributor

nice @dkuppitz - i can finally VOTE +1 🥇

better merge it through fast before someone sees a problem! 😈

@asfgit asfgit merged commit 906f244 into tp33 May 12, 2018
@asfgit asfgit deleted the TINKERPOP-1447 branch May 19, 2018 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants