Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix#8 the default value of keepgenerated is false only in JDK 6. #26

Merged
merged 1 commit into from
Nov 27, 2021

Conversation

hs536
Copy link
Contributor

@hs536 hs536 commented Jun 21, 2021

The default value for JSP keepgenerated should be false in JDK 6 or later, but only in JDK 6.

Signed-off-by: hs536 sawamura.hiroki@fujitsu.com

closes #8

@joakime
Copy link
Member

joakime commented Jun 21, 2021

With the next version of Jakarta EE (Jakarta EE 10), there is a strong chance that it will have Java 11 as the minimum supported version of Java (it's being voted on right now).
This JVM specific concept in EmbeddedServletOptions will probably just be removed at that point.

…JDK version.

Signed-off-by: hs536 <sawamura.hiroki@fujitsu.com>
@hs536
Copy link
Contributor Author

hs536 commented Sep 7, 2021

@joakime @arjantijms
This PR was finally fixed by removing the special handling with JDK 6. Is there anything else I can do?

Copy link
Member

@joakime joakime left a comment

Choose a reason for hiding this comment

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

@markt-asf you good with this change?

@markt-asf
Copy link

I have no objection but my opinion shouldn't count much as I'm not involved in the Jakarta implementation work at Eclipse.
On a mostly unrelated topic, I did notice that WASP is still using the org.apache.jasper package. Changing the package name is long overdue in my view.

@joakime
Copy link
Member

joakime commented Sep 9, 2021

On a mostly unrelated topic, I did notice that WASP is still using the org.apache.jasper package. Changing the package name is long overdue in my view.

Yeah, that should be a new issue / pr IMO

@joakime
Copy link
Member

joakime commented Sep 9, 2021

@markt-asf opened issue about package namespace as #29

@joakime
Copy link
Member

joakime commented Sep 9, 2021

@arjantijms you have any objections with this PR?

@arjantijms
Copy link
Contributor

@joakime it looks fine, indeed no need to keep anything JDK 6 around.

A tiny remark, and that's here:

private boolean keepGenerated = false;

false is already the default for a boolean field, so there's no need to set it explicitly.

@hs536
Copy link
Contributor Author

hs536 commented Sep 10, 2021

false is already the default for a boolean field, so there's no need to set it explicitly.

Yes, I think the default values are defined clearly because they correspond to jasper options.

/**
* Do you want to keep the generated Java files around?
*/
private boolean keepGenerated = false;
/**
* If class files are generated as byte arrays, should they be saved to disk at the end of compilations?
*/
private boolean saveBytecode = false;
/**
* Should white spaces between directives or actions be trimmed?
*/
private boolean trimSpaces = false;

@hs536
Copy link
Contributor Author

hs536 commented Nov 1, 2021

@joakime Is there anything else I can do to merge this fix?

@arjantijms
Copy link
Contributor

It looks good, thanks! We may pretty soon indeed set the defaults to JDK 11.

@arjantijms arjantijms added this to the 3.0.2 milestone Nov 27, 2021
@arjantijms arjantijms merged commit d2b0a95 into eclipse-ee4j:master Nov 27, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why just make a special judgment on JDK6
4 participants