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

JDK-8254721: Improve support for conditionally generated files #674

Closed

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Oct 15, 2020

The overall pages is very dynamic and depends on a lot of factors, such as the input files and the command-line options. Some of the summary files depend on the content of the classes being documented, such as:

  • any constant values declared in the files
  • any deprecated elements
  • any serializable classes
  • any classes whose description references system properties

This change is a step towards handling these files in a uniform manner.

There is a new test to test the consistency of the new behavior..


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254721: Improve support for conditionally generated files

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/674/head:pull/674
$ git checkout pull/674

Improve support for conditionally generated files
@jonathan-gibbons jonathan-gibbons marked this pull request as draft Oct 15, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 15, 2020

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2020

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • javadoc

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.

@openjdk openjdk bot added the javadoc label Oct 15, 2020
@jonathan-gibbons jonathan-gibbons marked this pull request as ready for review Oct 16, 2020
@openjdk openjdk bot added the rfr label Oct 16, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 17, 2020

Webrevs

@hns
hns approved these changes Oct 19, 2020
Copy link
Member

@hns hns left a comment

Looks good, Jon. The inline comments all refer to minor issues.

// System Properties
if (configuration.conditionalPages.contains(HtmlConfiguration.PageKind.SYSTEM_PROPERTIES)) {
section = newHelpSection(contents.systemPropertiesLabel);
Content constantsBody = getContent("doclet.help.systemProperties.body",

This comment has been minimized.

@hns

hns Oct 19, 2020
Member

Not an actual problem, but constantsBody is the name used for the constant values page. Should be either a generic name or one hinting at the system properties content.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Oct 19, 2020
Author Contributor

Good catch; will fix.

*/
// Note: this should (eventually) be merged with Navigation.PageMode,
// which performs a somewhat similar role
public enum PageKind {

This comment has been minimized.

@hns

hns Oct 19, 2020
Member

Maybe call it ConditionalPage and change comment to "Kinds of conditional pages" to make purpose/scope clearer?

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Oct 19, 2020
Author Contributor

That would be a reasonable short term fix, but I was hoping to merge with Navigation.PageMode since both are enums for the different kinds of pages we generate. But I have not yet found the sweet spot for where to put this enum: it may be best for it to end up as a top-level enum in the formats.html package. Or, we avoid merging them, and rename PageKind as you suggest.

depr.generateDeprecatedListFile(
new DeprecatedAPIListBuilder(configuration));
DeprecatedAPIListBuilder deprAPI = configuration.deprecatedAPIListBuilder;
if (deprAPI != null && !deprAPI.isEmpty()) {

This comment has been minimized.

@hns

hns Oct 19, 2020
Member

Just a note regarding consistency: everywhere else you are using configuration.conditionalPages.contains(HtmlConfiguration.PageKind.DEPRECATED) for this check.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Oct 19, 2020
Author Contributor

Yes, thanks; noted; I guess this is a hangover from before I added the PageKind.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@jonathan-gibbons 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:

8254721: Improve support for conditionally generated files

Reviewed-by: hannesw

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 28 new commits pushed to the master branch:

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 19, 2020
@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Oct 19, 2020

Responded to all review feedback, and latest version builds and tests. Will integrate.

/integrate

@openjdk openjdk bot closed this Oct 19, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@jonathan-gibbons Since your change was applied there have been 29 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit cb5cdd7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jonathan-gibbons jonathan-gibbons deleted the jonathan-gibbons:8254721-optional-pages branch Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants