Copilot commented on code in PR #7466:
URL: https://github.com/apache/ignite-3/pull/7466#discussion_r2719258520
##########
modules/platforms/build.gradle:
##########
@@ -267,6 +258,31 @@ private String simpleProjectVersion(String version) {
return version
}
+/**
+ * Converts the version to the python version format.
+ * See https://packaging.python.org/en/latest/discussions/versioning/
+ */
+private String pythonProjectVersion(String version) {
+ def (simple, addition) = version.tokenize('-')
+ if (addition == null) {
+ return simple
+ }
+
+ def pythonAddition = addition
+ .toLowerCase()
+ .replace("alpha", "a")
+ .replace("beta", "b")
+ .replace("patch", "p")
+
+ if (addition.startsWith("alpha")
+ || addition.startsWith("beta")
+ || addition.startsWith("patch"))
Review Comment:
The conditional checks use the original `addition` variable instead of the
lowercased `pythonAddition` variable. This means the checks will fail for
uppercase pre-release identifiers (e.g., "ALPHA", "BETA", "PATCH"). The
condition should check `pythonAddition.startsWith()` instead of
`addition.startsWith()` since the replacements were made on the lowercased
version.
```suggestion
def lowercasedAddition = addition.toLowerCase()
def pythonAddition = lowercasedAddition
.replace("alpha", "a")
.replace("beta", "b")
.replace("patch", "p")
if (lowercasedAddition.startsWith("alpha")
|| lowercasedAddition.startsWith("beta")
|| lowercasedAddition.startsWith("patch"))
```
##########
modules/platforms/build.gradle:
##########
@@ -267,6 +258,31 @@ private String simpleProjectVersion(String version) {
return version
}
+/**
+ * Converts the version to the python version format.
+ * See https://packaging.python.org/en/latest/discussions/versioning/
+ */
+private String pythonProjectVersion(String version) {
+ def (simple, addition) = version.tokenize('-')
+ if (addition == null) {
+ return simple
+ }
+
+ def pythonAddition = addition
+ .toLowerCase()
+ .replace("alpha", "a")
+ .replace("beta", "b")
+ .replace("patch", "p")
+
+ if (addition.startsWith("alpha")
+ || addition.startsWith("beta")
+ || addition.startsWith("patch"))
+ {
Review Comment:
The opening brace is placed on its own line, which is inconsistent with the
style used elsewhere in this file where braces are typically on the same line
as the conditional statement. For consistency, consider moving the opening
brace to the same line as the last condition or the first condition.
```suggestion
|| addition.startsWith("patch")) {
```
##########
modules/platforms/build.gradle:
##########
@@ -267,6 +258,31 @@ private String simpleProjectVersion(String version) {
return version
}
+/**
+ * Converts the version to the python version format.
+ * See https://packaging.python.org/en/latest/discussions/versioning/
+ */
+private String pythonProjectVersion(String version) {
+ def (simple, addition) = version.tokenize('-')
Review Comment:
The `tokenize('-')` method splits on all occurrences of '-', not just the
first one. If a version string contains multiple dashes (e.g.,
"3.2.0-alpha-1"), the destructuring assignment will fail because it expects
exactly 2 elements but will receive more. Consider using `split('-', 2)` or
handling the first dash only with `indexOf('-')` to safely handle multi-dash
version strings.
```suggestion
def dashIndex = version.indexOf('-')
def simple
def addition
if (dashIndex == -1) {
simple = version
addition = null
} else {
simple = version.substring(0, dashIndex)
addition = version.substring(dashIndex + 1)
}
```
##########
modules/platforms/build.gradle:
##########
@@ -267,6 +258,31 @@ private String simpleProjectVersion(String version) {
return version
}
+/**
+ * Converts the version to the python version format.
+ * See https://packaging.python.org/en/latest/discussions/versioning/
+ */
+private String pythonProjectVersion(String version) {
+ def (simple, addition) = version.tokenize('-')
+ if (addition == null) {
+ return simple
+ }
+
+ def pythonAddition = addition
+ .toLowerCase()
+ .replace("alpha", "a")
+ .replace("beta", "b")
+ .replace("patch", "p")
+
+ if (addition.startsWith("alpha")
+ || addition.startsWith("beta")
+ || addition.startsWith("patch"))
+ {
+ return simple + pythonAddition
+ }
+ return simple + '.' + pythonAddition
+}
Review Comment:
The version conversion logic does not handle release candidate versions
properly. According to PEP 440, release candidates should be formatted as
"3.2.0rc1" (without a dot separator), similar to alpha and beta. However,
versions like "3.2.0-rc1" will be converted to "3.2.0.rc1" (with a dot), which
is not the standard format. Consider adding support for "rc" by including it in
the replace logic and the startsWith check.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
