rpuch commented on code in PR #6299:
URL: https://github.com/apache/ignite-3/pull/6299#discussion_r2230357903
##########
modules/platforms/dotnet/Apache.Ignite.Tests/ErrorGroupTests.cs:
##########
@@ -212,7 +212,8 @@ public void TestExceptionProperties()
return groupClass
.GetFields()
- .Where(x => x.Name != "GroupCode" && x.Name !=
"GroupName" && x.Name !="ErrorPrefix")
+ .Where(x => x.Name != "GroupCode" && x.Name !=
"GroupName" && x.Name != "ErrorPrefix" &&
+
x.GetCustomAttributes(typeof(ObsoleteAttribute), false).Length == 0)
Review Comment:
Why is this change needed?
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/processor/ErrorCodeGroupProcessor.java:
##########
@@ -140,16 +142,30 @@ private Object visitErrorCodeField(VariableTree
variableTree, Trees trees) {
var initializer = variableTree.getInitializer();
var name = variableTree.getName().toString();
try {
- // example: args = {"(short) 1"} as List<ExpressionTree>.
- var args = ((MethodInvocationTree) initializer).getArguments();
- // example: expr = "(short) 1" as TypeCastTree.
- var expr = ((TypeCastTree) args.get(0)).getExpression();
- // example: if expr is "(short) (1)" we should remove
parentheses
- if (expr instanceof ParenthesizedTree) {
- expr = ((ParenthesizedTree) expr).getExpression();
+ if
(MethodInvocationTree.class.isAssignableFrom(initializer.getClass())) {
+ // example: args = {"(short) 1"} as List<ExpressionTree>.
+ var args = ((MethodInvocationTree)
initializer).getArguments();
+ // example: expr = "(short) 1" as TypeCastTree.
+ var expr = ((TypeCastTree) args.get(0)).getExpression();
+ // example: if expr is "(short) (1)" we should remove
parentheses
+ if (expr instanceof ParenthesizedTree) {
+ expr = ((ParenthesizedTree) expr).getExpression();
+ }
+ // example: extract 1 from "(short) 1" expression.
+ this.descriptor.errorCodes.add(new ErrorCode((Integer)
((LiteralTree) expr).getValue(), name));
+ } else if
(IdentifierTree.class.isAssignableFrom(initializer.getClass())) {
+ boolean hasDeprecated =
variableTree.getModifiers().getAnnotations().stream()
+ .anyMatch(annotation ->
annotation.toString().contains("@Deprecated"));
Review Comment:
Do we have information about the package of the annotation here? What does
`annotation.toString()` produce?
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/processor/ErrorCodeGroupDescriptor.java:
##########
@@ -45,5 +45,19 @@ public static class ErrorCode {
public String name;
}
+ /**
+ * Class that holds info about one (deprecated) alias for an error code .
+ */
+ public static class DeprecatedAlias {
+ public final String name;
+ public final String identifier;
Review Comment:
It seems that `name` is the alias itself (i.e. the alternative name) and the
`identifier` is the original name that is being aliased. But I learned this
from reading the code that instantiates this class, and from its code alone
this is not clear.
I suggest to rename the fields (along with constructor params, of course):
1. `name` -> `alias`
2. `identifier` -> `aliasedName`
This will make it obvious which is which.
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/generators/CsharpGenerator.java:
##########
@@ -113,6 +114,11 @@ private void
generateErrorGroupClass(ErrorCodeGroupDescriptor descriptor) throws
}
}
+ for (int i = 0; i < descriptor.deprecatedAliases.size(); i++) {
Review Comment:
How about changing this to an enhanced for loop (aka foreach)? We don't seem
to need the index, and the code will become a little more concise and a little
less error-prone
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/generators/CsharpGenerator.java:
##########
@@ -113,6 +114,11 @@ private void
generateErrorGroupClass(ErrorCodeGroupDescriptor descriptor) throws
}
}
+ for (int i = 0; i < descriptor.deprecatedAliases.size(); i++) {
+ DeprecatedAlias deprecatedAlias =
descriptor.deprecatedAliases.get(i);
+ generateDeprecatedAlias(deprecatedAlias.name,
deprecatedAlias.identifier);
Review Comment:
Let's just pass `deprecatedAlias` as one argument, there doesn't seem to be
a need to deconstruct it here
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/generators/CsharpGenerator.java:
##########
@@ -121,4 +127,15 @@ private void generateErrorCode(String name, int code)
throws IOException {
line(String.format(" public const int %s = (GroupCode <<
%d) | (%d & 0xFFFF);", transfromErrorCodeName(name), groupShift,
code));
}
+
+ private void generateDeprecatedAlias(String name, String identifier)
throws IOException {
+ String transformedName = transfromErrorCodeName(name);
+ String transformedIdentifier = transfromErrorCodeName(identifier);
+
+ line();
+ line(String.format(" /// <summary> %s is deprecated. Use %s
instead. </summary>", transformedName,
Review Comment:
Is 'deprecated' the expected wording in .Net world if the attribute is
called `Obsolete`?
##########
modules/error-code-annotation-processor/src/main/java/org/apache/ignite/internal/error/code/processor/ErrorCodeGroupProcessor.java:
##########
@@ -140,16 +142,30 @@ private Object visitErrorCodeField(VariableTree
variableTree, Trees trees) {
var initializer = variableTree.getInitializer();
var name = variableTree.getName().toString();
try {
- // example: args = {"(short) 1"} as List<ExpressionTree>.
- var args = ((MethodInvocationTree) initializer).getArguments();
- // example: expr = "(short) 1" as TypeCastTree.
- var expr = ((TypeCastTree) args.get(0)).getExpression();
- // example: if expr is "(short) (1)" we should remove
parentheses
- if (expr instanceof ParenthesizedTree) {
- expr = ((ParenthesizedTree) expr).getExpression();
+ if
(MethodInvocationTree.class.isAssignableFrom(initializer.getClass())) {
+ // example: args = {"(short) 1"} as List<ExpressionTree>.
+ var args = ((MethodInvocationTree)
initializer).getArguments();
+ // example: expr = "(short) 1" as TypeCastTree.
+ var expr = ((TypeCastTree) args.get(0)).getExpression();
+ // example: if expr is "(short) (1)" we should remove
parentheses
+ if (expr instanceof ParenthesizedTree) {
+ expr = ((ParenthesizedTree) expr).getExpression();
+ }
+ // example: extract 1 from "(short) 1" expression.
+ this.descriptor.errorCodes.add(new ErrorCode((Integer)
((LiteralTree) expr).getValue(), name));
+ } else if
(IdentifierTree.class.isAssignableFrom(initializer.getClass())) {
+ boolean hasDeprecated =
variableTree.getModifiers().getAnnotations().stream()
+ .anyMatch(annotation ->
annotation.toString().contains("@Deprecated"));
+ if (!hasDeprecated) {
+ ex = new
ErrorCodeGroupProcessorException(String.format("Alias %s must be marked as
@Deprecated", name));
+ } else {
+ var identifier = ((IdentifierTree)
initializer).getName().toString();
+
+ descriptor.deprecatedAliases.add(new
DeprecatedAlias(name, identifier));
+ }
+ } else {
+ ex = new
ErrorCodeGroupProcessorException(String.format("AST parsing error: %s",
variableTree));
Review Comment:
Please improve the error message to be more specific: that is, it should
probably tell that it expected a method call or an identifier as an
initializer, but it got `<what it actually got>`
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
