-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8276124: Provide snippet support for properties files #6397
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
Conversation
After JDK-8267636 has been integrated, jdk.javadoc is compiled using JDK 17. So we can finally use sealed classes where they were originally envisioned.
8266666 didn't test this option. This commit adds such a test with the "@bug 8266666" declaration. The test improves code coverage of jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet as follows: %method %block %branch %line -----------+-----------+----------+------------ before 100%(11/11) 88%(92/104) 84%(81/96) 88%(128/145) after 100%(11/11) 90%(94/104) 89%(86/96) 89%(130/145) Note that the table might become outdated due to rebases.
- subclasses JavadocTester to provide reusable check and utility methods that will be shared by multiple snippet test classes - fixes a stupid regex mistake: \s -> \\s (filed a bug https://youtrack.jetbrains.com/issue/IDEA-279378) - re-orders some imports
The new test improves code coverage of jdk.javadoc.internal.doclets.toolkit.taglets.snippet as follows:
%method %block %branch %line
-----------+------------+------------+------------
before 70%(80/114) 76%(310/407) 65%(178/273) 81%(413/508)
after 82%(94/114) 84%(344/407) 73%(202/273) 90%(459/508)
Note that the table might become outdated due to rebases.
- removes dead code - uses autogenerated getters instead of field access for records
Changes tests names to match the terminology change that happened long ago: redundant snippets -> hybrid snippets.
A test may exercise both markup and tag syntax. Generally speaking, a test may be a part of multiple hierarchies. To model that, tests should be organized using tags (i.e. labels). A name component can be such a tag.
This commit immediately removes some amount of repetition.
Ideally, these tests should be implemented using hybrid snippets, to show how inline and external parts differ in treatment of */ and \uxxxx.
Increases test coverage for the "region" tag attribute. Fixes the bug where the valueless "region" attribute caused a crash (NPE).
Unifies error handling in the taglet by extracting an internal method and wrapping that method in try-catch. Fixes a few bugs where the attribute unexpectedly lacks value.
Covers extra 5 lines of SnippetTaglet: [244, 248]
Covers extra 2 lines of MarkupParser: [101, 102]
|
👋 Welcome back prappo! A progress list of the required criteria for merging this PR into |
|
@pavelrappo The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
jonathan-gibbons
left a comment
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 think you can simplify the code by reducing CoarseParser down to a language-specific regular expression with "standard" named groups for the payload and markup.
I'd also recommend not using an enum for Language to leave open the possibility of easily/dynamically adding support for additional languages.
| @Override | ||
| public int payloadEnd() {return m.end(1);} | ||
|
|
||
| @Override | ||
| public int markupStart() {return m.start(3);} |
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.
Suggest using named groups
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'll answer this below.
| private final Matcher m = Pattern.compile("^(.*)(//(\\s*@\\s*\\w+.+?))$").matcher(""); | ||
|
|
||
| @Override | ||
| public boolean parse(String line) {return m.reset(line).matches();} |
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.
Given that matchers are lightweight objects, what is the benefit of using a pre-allocated matcher and reset as compared to a temporary Matcher object? Is it just style, or is there something more?
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.
Mostly style, I think.
| // Without this interface regular expressions that describe comments would have to | ||
| // to be standardized or needlessly complex. | ||
|
|
||
| private String eolMarker; | ||
| private Matcher markedUpLine; | ||
| private interface CoarseParser { | ||
| boolean parse(String line); | ||
| int payloadEnd(); | ||
| int markupStart(); | ||
| } |
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'm surprised/disappointed that this may be necessary, although I accept there is a notable difference between "whole-line comments" and "end-of-line comments". Is it possible to reduce CoarseParser down to a language-specific regular expression with named groups for the payload and markup, such that the regular expression could be a property of the Language object?
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'm going to answer both of your comments, this and that one above:
I think you can simplify the code by reducing
CoarseParserdown to a language-specific regular expression with "standard" named groups for the payload and markup.
Regular expressions with named groups were the initial design. However, I changed it to CoarseParser halfway through the implementation, when saw how bulky the named regex were becoming. As you also note, "end-of-line comments" and "comment lines" differ. I couldn't quickly come up with a regex that accounts for both of them.
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.
You may be somewhat missing the point I was trying to make.
You have two impls of CoarseParser, both of which contain a regular expression for the parsing, hidden inside their private matcher field.
The only other functionality of CoarseParser is payloadEnd and markupStart.
My suggestion is to start by updating each of the regex with named groups for the payload and markup parts of the line, such that you can derive payloadEnd and markupStart from the appropriate named groups.
At that point, the only thing unique about the impls of CoarseParser is their regex, and that regex could become a property of the Language object.
As you also note, "end-of-line comments" and "comment lines" differ. I couldn't quickly come up with a regex that accounts for both of them.
To be clear, I am not suggesting a single regex. I am suggesting a regex per supported language.
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 abstracted out the mechanics behind the CoarseParser precisely because I couldn't come up with a simple way to derive payloadEnd and markupStart using only groups, be they named or otherwise. My regex fu is not strong enough. If you could find a way that does not look too ugly and passes the added TestLangProperties test, be my guest.
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 think the following regexes with named groups should yield the same results as yours:
"^(?<payload>.*)//(?<markup>\\s*@\\s*\\w+.+?)$"
"^(?<payload>[ \t]*([#!].*)?)[#!](?<markup>\\s*@\\s*\\w+.+?)$"
(Note that I didn't try running them through the test, maybe I should have :)
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.
Above regexes do pass the test, but I just saw (too late) that Jon posted a generic (language agnostic) solution below in the main conversation thread, which probably makes above solution obsolete.
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.
Your regexes are good. To me, they are conceptually simpler than those proposed by Jon. Both yours and Jon's are simpler than what I initially had.
| */ | ||
| public class SnippetTaglet extends BaseTaglet { | ||
|
|
||
| public enum Language { |
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.
The idea of a Language object is good, but using an enum precludes easy extensibility.
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.
We'll figure out the extensibility when the need comes. The language of a snippet defines both the comment marker and the comment type. For now, an alternative to that enum would be a boolean flag, which is less readable.
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.
All I'm suggesting is to make Language a class, with a public constructor and (for now) some standard static members. In other words, plan for the possibility of additional languages, by not limiting the code to an enum at this time.
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.
Let's figure out the CoarseParser and "plain text" issues first and then circle back to this discussion. That said, we need to be mindful of time since we're close to RDP 1.
| /* | ||
| * @test | ||
| * @bug 8266666 | ||
| * @summary Implementation for snippets | ||
| * @library /tools/lib ../../lib | ||
| * @modules jdk.compiler/com.sun.tools.javac.api | ||
| * jdk.compiler/com.sun.tools.javac.main | ||
| * jdk.javadoc/jdk.javadoc.internal.tool | ||
| * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder builder.ClassBuilder | ||
| * @run main TestLangProperties | ||
| */ |
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.
move before imports
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.
| */ | ||
| public class TestLangProperties extends SnippetTester { | ||
|
|
||
| private final ToolBox tb = new ToolBox(); |
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.
Suggest moving to SnippetTester (suggested in dependent PR)
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.
|
|
||
| private final ToolBox tb = new ToolBox(); | ||
|
|
||
| private TestLangProperties() {} |
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.
Just curious: why force this?
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.
You mean, why make that constructor private? No reason. This file started as a copy-paste. Fixed.
- Hoist the tb (toolbox) field to SnippetTester - Use use default ctors instead of private no-arg ctors for test classes - Re-implement SnippetTester.checkOutputEither on top of JavadocTester.OutputChecker.checkAnyOf - Follow convention for jtreg comment - Improve method names with underscores
|
The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout 8273544
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@pavelrappo this pull request can not be integrated into git checkout 8276124
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
|
|
Apologies to those who read my previous reply to Jon on the javadoc-dev mailing list. Apparently, comment order on GitHub differs from that of the mailing list. So my relative references such as "above" and "below" are broken. |
|
It would be nice to define/support the markup characteristics of "plain text" snippets, where "plain text" might be a stand-in for commands, or stack traces, or other less common formats. |
If we are to do it, we should discuss and implement it quickly, before RDP 1. We would still be able to do it later, before RC, but only with approval. Here are some questions to discuss.
|
Maybe too late for now, allow a new attribute to specify the markup "prefix" ( |
Introducing such an attribute just for plain text seems ad-hoc. This is because there are dependencies between these three elements that make them not completely orthogonal:
|
|
Here's a suggestion for a method to generate a pattern to do the coarse parsing of snippet lines, to see if they contain markup comments. The
The pattern contains named groups for the |
Uses named groups and positive lookahead to make both regexes uniform in respect to accessing the components of a match.
Substitutes regex for CoarseParser and its implementations.
Cleans up.
Shortens regexes and makes them use fewer concepts (no lookahead).
|
Thank you, Jon and Hannes. I believe this PR fully addresses the linked JBS issue. I would prefer to leave these discussions for later, unless any of them are showstoppers, and integrate the change ASAP:
|
| testPositive(base, testCases); | ||
| } | ||
|
|
||
| // @replace on a blank line will not do anything bad! |
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.
What is the purpose of this comment? Is this used as snippet source somewhere?
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.
This is an artifact of me thinking of three interdependent PRs simultaneously. A marginal note, if you will. This line should be translated into a new test in the upcoming PR, #6461. I'll move it there, thanks.
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.
Approved, for now. It's good to have simplified CoarseParser down to regular expressions.
I look forward to the next step of cleanup (for 19?) where the design contained here can be extended to additional languages, perhaps dynamically (in some fashion TBD).
The regular expressions are a function on the comment character(s), and whether the comment is a "line comment" or "end-of-line comment". These properties could be described in members of Language and the corresponding regular expressions generated by the following method:
Pattern getPattern(String cmt, boolean eol) {
Pattern payload = eol
? Pattern.compile("(?<payload>.*)")
: Pattern.compile("(?<payload>[ \t]*(" + cmt + ".*)?)");
Pattern prefix = Pattern.compile(cmt + "(?=[ \t]*@[a-z]+\\b)");
Pattern markup = Pattern.compile("(?<markup>.*)");
return Pattern.compile(payload.pattern() + prefix.pattern() + markup.pattern());
}
Related, Language could usefully be converted from an enum to a record, and maybe have a cache-map of Map<Language, Pattern> to reduce the costs of creating those patterns.
|
@pavelrappo This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 51 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/contributor add jjg |
|
@pavelrappo |
|
@pavelrappo |
|
/integrate |
|
Going to push as commit e785f69.
Your commit was automatically rebased without conflicts. |
|
@pavelrappo Pushed as commit e785f69. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The initial integration (JDK-8266666) of JEP 413 did not support properties files. This commit rights that wrong.
Progress
Issue
Reviewers
Contributors
<jjg@openjdk.org><hannesw@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6397/head:pull/6397$ git checkout pull/6397Update a local copy of the PR:
$ git checkout pull/6397$ git pull https://git.openjdk.java.net/jdk pull/6397/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6397View PR using the GUI difftool:
$ git pr show -t 6397Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6397.diff