devhub-tud / devhub

DevHub is a software system designed to give students a simple practical introduction into modern software development.
15 stars 8 forks source link

[WIP] add markdown support for comments, assignments and mardown files #391

Closed DouweKoopmans closed 8 years ago

DouweKoopmans commented 8 years ago

fixes #149

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 57.127% when pulling bb98ba9c80ec96a3023a197bfcaa3636f1e1cf22 on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 57.127% when pulling bb98ba9c80ec96a3023a197bfcaa3636f1e1cf22 on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

DouweKoopmans commented 8 years ago

don't know why coveralls says the coverage went up, i haven't written any tests yet

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.09%) to 57.041% when pulling cb13d4123b2af2777be40354f8bd0acf5b135bed on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 57.1% when pulling 63cdf9f139b284d8242d15ab91091b79c252fc85 on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.09%) to 57.042% when pulling 274ae7e5cd439485aaf38f0e34b07705f04ec8e5 on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.01%) to 56.966% when pulling e53554c7f233e6c35ad8896418bdfacd8a1c4b7c on MarkdownRendering into 546ac5710f069ffd304344a6a6a8976613d3e545 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 57.051% when pulling 7335de63cbb2a85670b22ac35f815d16b871e498 on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.

jwgmeligmeyling commented 8 years ago

I have added one commit to the PR. This commit addresses issues related to static state. In Devhub we provide dependecies through dependency injection. Static access is considered a bad practise. This commit addresses that problem, and provides the PegDownParser and MarkDownParser by DI.

Concurrent access to the PegDownParser is prevented by binding it to the RequestScope. No synchronized access is needed, so we keep our concurrency high.

As the TemplateEngine was accessed as a singleton, and it depends on the PegDownParser, access to the TemplateEngine had to be transformed to RequestScoped, which is done by using Guice Providers.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 56.929% when pulling f440fb77d925437b005df267e3971a8319ed77ef on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.

jwgmeligmeyling commented 8 years ago

This implementation suffers from the problem that Pegdown also escapes HTML code, but only within code blocks. Therefore code blocks are now escaped twice, resulting in strange output. We should however run the request over an escaper, as Pegdown only escapes code blocks, and this would make us vulnerable to XSS.

See: https://github.com/sirthias/pegdown/issues/236

jwgmeligmeyling commented 8 years ago

The ideal way to have objects that write to the Freemarker output is to use a directive. This commit implements such a directive on the existing Markdown parser object. With this change, [#noescape] tags around the Markdown blocks are no longer required. Furthermore, we do not need the BeansWrapper anymore.

The current syntax is:

[@MarkDownParser message=comment.content/]

Now lets keep an eye on the duplicate escaping issue. Imho no show stopper, as most people will put Java code in the code blocks in our use case anyway.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 56.935% when pulling d4883e51608cb871277f19c4408d6eef634bc346 on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.

DouweKoopmans commented 8 years ago

unfortunately quotation marks are being escaped, so this creates weird output when working with java strings.

jwgmeligmeyling commented 8 years ago

Ah thats a shame... Maybe we should fix Pegdown?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.09%) to 56.896% when pulling badcd96e44946e13ad781768167b0e983c20d7d5 on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 57.517% when pulling c5f5f23069e692382e2e3620faff550c01d90e29 on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.9%) to 57.753% when pulling 8e454b35d484dca1e594d9da52f67bc1d61fa3fa on MarkdownRendering into 70e815ac49b5ae57025d91fbd036fafcb6264192 on master.