vlsi commented on code in PR #125:
URL: https://github.com/apache/xalan-java/pull/125#discussion_r1399320217
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
Review Comment:
Could you clarify what is the purpose of explicitly querying
`xalan/target/classes/...`?
As far as I know, IDE includes `xalan/target/classes` into the classpath
anyway, so if the property file is there, then it would already be covered in
`getResourceAsStream()`.
I suggest adding a code comment that would clarify why this file is queried
in the first place
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
Review Comment:
Have you considered making the fields `final`?
##########
serializer/src/main/java/org/apache/xml/serializer/Version.java:
##########
@@ -20,33 +20,97 @@
*/
package org.apache.xml.serializer;
+
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Serializer release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information.</P>
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information.</P>
* @xsl.usage general
*/
public final class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xml/serializer/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"serializer/target/classes" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
+
+ private static boolean snapshot;
Review Comment:
It looks like `serializer/Version` logic duplicates `xalan/Version` can it
be shared?
`xalan` depends on `serializer` anyway, so it looks like `xalan` could just
reuse the logic from `serializer` instead of duplicating it.
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
+
+ private static boolean snapshot;
+
+ static {
+ readProperties();
+ parseVersionNumber();
+ }
+
+ private static void readProperties() {
+ Properties pomProperties = new Properties();
+ try (InputStream fromJar =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_JAR)) {
+ if (fromJar != null) {
+ pomProperties.load(fromJar);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ else {
+ try (FileInputStream fromFileSystem = new
FileInputStream(POM_PROPERTIES_FILE_SYSTEM)) {
+ pomProperties.load(fromFileSystem);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ }
+ }
+ catch (IOException e) {
+ new RuntimeException("Cannot read properties file to extract version
number information: ", e)
+ .printStackTrace();
Review Comment:
The error message on the stderr would be hard to relate to Xalan.
I would suggest use `java.util.logging` for the logging instead of
`System.err`. Then the users would be able to identify the source of the
message and silence it without recompiling Xalan.
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
+
+ private static boolean snapshot;
+
+ static {
+ readProperties();
+ parseVersionNumber();
+ }
+
+ private static void readProperties() {
+ Properties pomProperties = new Properties();
+ try (InputStream fromJar =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_JAR)) {
+ if (fromJar != null) {
+ pomProperties.load(fromJar);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ else {
+ try (FileInputStream fromFileSystem = new
FileInputStream(POM_PROPERTIES_FILE_SYSTEM)) {
+ pomProperties.load(fromFileSystem);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
Review Comment:
Can you clarify when this codepath could be triggered?
I cloned https://github.com/kriegaex/Maven_GenerateSourcesExample in IDEA
2023.2.3, and sure it adds `target/classes` when executing `Main`
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
+
+ private static boolean snapshot;
+
+ static {
+ readProperties();
+ parseVersionNumber();
+ }
+
+ private static void readProperties() {
+ Properties pomProperties = new Properties();
+ try (InputStream fromJar =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_JAR)) {
+ if (fromJar != null) {
+ pomProperties.load(fromJar);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ else {
+ try (FileInputStream fromFileSystem = new
FileInputStream(POM_PROPERTIES_FILE_SYSTEM)) {
+ pomProperties.load(fromFileSystem);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ }
+ }
+ catch (IOException e) {
+ new RuntimeException("Cannot read properties file to extract version
number information: ", e)
+ .printStackTrace();
+ }
+ }
+
+ private static void parseVersionNumber() {
+ Matcher matcher = Pattern.compile(VERSION_NUMBER_PATTERN).matcher(version);
+ if (matcher.find()) {
+ majorVersionNum = Integer.parseInt(matcher.group(1));
+ releaseVersionNum = Integer.parseInt(matcher.group(2));
+ if (matcher.group(3) == null) {
+ maintenanceVersionNum = Integer.parseInt(matcher.group(4));
+ }
+ else {
+ developmentVersionNum = Integer.parseInt(matcher.group(4));
+ }
+ snapshot = matcher.group(5) != null && !matcher.group(5).isEmpty();
+ }
+ else {
+ System.err.println(
+ "Cannot match version \"" + version + "\" " +
+ "against expected pattern \"" + VERSION_NUMBER_PATTERN + "\""
+ );
+ }
+ }
/**
* Get the basic version string for the current Xalan release.
- * Version String formatted like
+ * Version String formatted like
* <CODE>"<B>Xalan</B> <B>Java</B> v.r[.dd| <B>D</B>nn]"</CODE>.
*
- * Futurework: have this read version info from jar manifest,
- * pom.properties, and/or a file updated during maven build.
- *
* @return String denoting our current version
*/
public static String getVersion()
{
return getProduct()+" "+getImplementationLanguage()+" "
+getMajorVersionNum()+"."+getReleaseVersionNum()+"."
- +( (getDevelopmentVersionNum() > 0) ?
- ("D"+getDevelopmentVersionNum()) :
(""+getMaintenanceVersionNum()));
+ +( (getDevelopmentVersionNum() > 0) ?
+ ("D"+getDevelopmentVersionNum()) :
(""+getMaintenanceVersionNum()))
+ +(isSnapshot() ? "-SNAPSHOT" :"");
Review Comment:
Should it return the original version from the property file rather than
reconstruct it?
It looks like `return version` should yield the same result
##########
xalan/src/main/java/org/apache/xalan/Version.java:
##########
@@ -20,36 +20,99 @@
*/
package org.apache.xalan;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Properties;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
/**
* Administrative class to keep track of the version number of
* the Xalan release.
* <P>This class implements the upcoming standard of having
- * org.apache.project-name.Version.getVersion() be a standard way
- * to get version information. This class will replace the older
+ * org.apache.project-name.Version.getVersion() be a standard way
+ * to get version information. This class will replace the older
* org.apache.xalan.processor.Version class.</P>
- * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
+ * <P>See also: org/apache/xalan/res/XSLTInfo.properties for
* information about the version of the XSLT spec we support.</P>
* @xsl.usage general
*/
public class Version
{
+ private static final String POM_PROPERTIES_JAR =
"org/apache/xalan/version.properties";
+ private static final String POM_PROPERTIES_FILE_SYSTEM =
"xalan/target/classes/" + POM_PROPERTIES_JAR;
+ private static final String VERSION_NUMBER_PATTERN =
"^(\\d+)[.](\\d+)[.](D)?(\\d+)(-SNAPSHOT)?$";
+ private static final String NO_VERSION = "0.0.0";
+
+ private static String version = NO_VERSION;
+ private static int majorVersionNum;
+ private static int releaseVersionNum;
+ private static int maintenanceVersionNum;
+ private static int developmentVersionNum;
+
+ private static boolean snapshot;
+
+ static {
+ readProperties();
+ parseVersionNumber();
+ }
+
+ private static void readProperties() {
+ Properties pomProperties = new Properties();
+ try (InputStream fromJar =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_JAR)) {
+ if (fromJar != null) {
+ pomProperties.load(fromJar);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ else {
+ try (FileInputStream fromFileSystem = new
FileInputStream(POM_PROPERTIES_FILE_SYSTEM)) {
+ pomProperties.load(fromFileSystem);
+ version = pomProperties.getProperty("version", NO_VERSION);
+ }
+ }
+ }
+ catch (IOException e) {
+ new RuntimeException("Cannot read properties file to extract version
number information: ", e)
+ .printStackTrace();
+ }
+ }
+
+ private static void parseVersionNumber() {
+ Matcher matcher = Pattern.compile(VERSION_NUMBER_PATTERN).matcher(version);
+ if (matcher.find()) {
+ majorVersionNum = Integer.parseInt(matcher.group(1));
+ releaseVersionNum = Integer.parseInt(matcher.group(2));
+ if (matcher.group(3) == null) {
+ maintenanceVersionNum = Integer.parseInt(matcher.group(4));
+ }
+ else {
+ developmentVersionNum = Integer.parseInt(matcher.group(4));
+ }
+ snapshot = matcher.group(5) != null && !matcher.group(5).isEmpty();
+ }
+ else {
+ System.err.println(
+ "Cannot match version \"" + version + "\" " +
+ "against expected pattern \"" + VERSION_NUMBER_PATTERN + "\""
+ );
Review Comment:
I suggest using `java.util.logging` instead of `System.err`.
The generic message `Cannot match version ...` would be hard to relate to
Xalan, and it would be virtually impossible to silence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For additional commands, e-mail: dev-h...@xalan.apache.org
