Support all kind of tags in DefaultDocWorkUnitHandler #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
=========================================
Coverage 74.08% 74.08%
Complexity 517 517
=========================================
Files 21 21
Lines 1999 1999
Branches 412 412
=========================================
Hits 1481 1481
Misses 346 346
Partials 172 172
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@magicDGS A couple of long comments on this one. Can you read through and let me know if I'm misunderstanding what you're trying to achieve with this. Either way I think we need to improve the tests for this stuff (tags), either as part of this PR or separately. More incentive for me to get to the mock framework PR.
| @@ -243,7 +243,7 @@ protected void addHighLevelBindings(final DocWorkUnit workUnit) | |||
| */ | |||
| protected void addCustomBindings(final DocWorkUnit currentWorkUnit) { | |||
| final String tagFilterPrefix = getTagPrefix(); | |||
| Arrays.stream(currentWorkUnit.getClassDoc().inlineTags()) | |||
| Arrays.stream(currentWorkUnit.getClassDoc().tags()) | |||
There was a problem hiding this comment.
Although I agree there is a problem with how we're processing the tags, I'm not sure this fixes the problem (or maybe I don't understand what you're trying to achieve here). The custom tags feature was intended to allow customizations that could be used from within the Freemarker template; for example, GATK3 tools had a "walker type" annotation, but GATK4 doesn't. So this was an attempt to allow that kind of functionality to be replaced with a tag, i.e.: "{@gatk:WalkerType ReadWalker}. We're not actually depending on that in GATK at the moment, but thats how I intended the feature would be used.
AFAICT, this change breaks that (sadly, it appears we don't have a good test for it). With this change, an embedded tag such as "{@gatk:WalkerType ReadWalker}" wouldn't be recognized anymore (that is, it would not be returned from the tags() method). For it to be recognized, it would have to be specified without the "{}" delimiters. But if the tag is written that way, the "text" of the tag will include everything in the comment up to start of the next tag, or to the end of the comment if no other tags exist. So I don't think you could use it to categorize a walker type.
There was a problem hiding this comment.
A couple of other things I noticed:
- I think we would want to be consistent with tag handling both here and for the summary, which still looks at inline tags.
- Even though TestArgumentContainer has an embedded custom tag, we don't actually have a test to make sure it gets propagated to the FreeMarker map. GATK has an integration test that includes a walkertype tag in PrintReads, and the GATK FreeMarker template uses it, but its only a smoke test - it doesn't actually look at the output. When I run it with this change, the tag no longer gets propagated.
- I was a bit inconsistent in how I handled the custom prefix in the code: DefaultDocWorkUnitHandler.getTagPrefix never returns null; if no custom tag is defined it returns "@.".
There was a problem hiding this comment.
My aim here is to have tags similar to the javadoc @see, but customized. This will allow to remove from the help text this tags and even include them in the normal javadoc. For example:
/**
* This is the description of the documentation.
* @barclay.type simple
* @author magicDGS
* @see OtherClass
*/
@DocumentedFeature
public class MyDocumentedClass {}Maybe my change is not working (which is wierd, because it mimics a hack that I did in a custom project to include this feature), but my aim is to populate the "barclay.type" in the JSON, but ignoring it in the documentation text description. In Barclay, the test for round trip is testing the JSON, but the tags are not populated there anyway (I haven't change the expected files); we should either have a test JSON object or implement mocking tests for all this functionality.
I also notice another problem with the tags: if there are repeated tags, they are overridden. For instance, I have the situation when I want to add custom tags with "notes", "warnings", "caveats" and then populate them into the JSON to format them as a list. That's actually why I started with this: I do not want to have them embedded in the description (in the format {@doc.warning be careful}), but being able to process them with the @doc.warning be careful format to remove them from the doc test. Nevertheless, the problem is when two warnings should be separated, and then one overrides the other.
Maybe we should discuss about the tag-support and how it should look like in a different issue, and I should close this PR til we agree on the final shape of this functionality...
There was a problem hiding this comment.
@magicDGS I think your example above will work as long as there is no other text in between these two lines:
* @barclay.type simple
* @author magicDGS
But I think it would break if it looked like this:
* @barclay.type simple
* Other comment included in between tags
* @author magicDGS
In that case, the custom @barclay.type tag's text will include everything up to the start of the next comment: "simple * Other comment included in between tags".
There was a problem hiding this comment.
Also, I realized from looking into this more, and from looking at the GATK beta doc output, that there is a bug in Barclay docgen where in some cases we don't include javadoc after the first embedded tag, at least in some cases. I'll try to fix that ASAP since its fairly high priority. In the meantime, lets leave this PR open for the short term so we can retain this discussion thread, even though we'll probably replace it with another PR with a more comprehensive fix.
There was a problem hiding this comment.
Actually that's a normal javadoc processing feature. These kind of tags (@see, @author, etc.) are suppose to be at the end of the javadoc instead of embedded in the text. That's why embedded tags such as @link have the curly-brackets syntax.
I think that the fix is to point out that this is like that... and actually IntelliJ re-format the javadoc putting them at the end, so I think that it's fair enough to keep it as it is now...
Implementation of #69