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.

Sign up for GitHub

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

Support all kind of tags in DefaultDocWorkUnitHandler #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support all kind of tags in DefaultDocWorkUnitHandler #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a3ebbc
Make public getTagPrefix
magicDGS Jun 6, 2017
80cc41d
Add support for all kind of tags to look up for custom ones
magicDGS Jun 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/org/broadinstitute/barclay/help/DefaultDocWorkUnitHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@cmnbroad cmnbroad Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 "@.".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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".

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

.filter(t -> t.name().startsWith(tagFilterPrefix))
.forEach(t -> currentWorkUnit.setProperty(t.name().substring(tagFilterPrefix.length()), t.text()));
}
Expand Down Expand Up @@ -329,7 +329,8 @@ protected void addCommandLineArgumentBindings(final DocWorkUnit currentWorkUnit,
}
}

private String getTagPrefix() {
/** Gets the tag prefix as formatted in the javadoc; {@code null} if there is no user-defined prefix. */
public final String getTagPrefix() {
String customPrefix = getTagFilterPrefix();
return customPrefix == null ?
customPrefix : "@" + customPrefix + ".";
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/broadinstitute/barclay/help/TestArgumentContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* <li>This is point number 2</li>
* </ol>
* </p>
*
* @MyTag.other this tag is not inline and will not appear in the description text.
*/
@CommandLineProgramProperties(
summary = "Test tool summary",
Expand Down