keith-turner commented on code in PR #102:
URL: https://github.com/apache/accumulo-access/pull/102#discussion_r2907857622
##########
README.md:
##########
@@ -61,35 +61,35 @@ Add the library to your CLASSPATH. For Maven, use:
</dependencies>
```
-## Running the [Examples](examples/src/main/java)
+## Running the [Examples](modules/examples/src/main/java)
-To run
[AccessExample](examples/src/main/java/org/apache/accumulo/access/examples/AccessExample.java)
+To run
[AccessExample](modules/examples/src/main/java/org/apache/accumulo/access/examples/AccessExample.java)
```
mvn clean package
-java
--module-path=core/target/accumulo-access-core-1.0.0-SNAPSHOT.jar:examples/target/accumulo-access-examples-1.0.0-SNAPSHOT.jar
--module=accumulo.access.examples/org.apache.accumulo.access.examples.AccessExample
-java
--module-path=core/target/accumulo-access-core-1.0.0-SNAPSHOT.jar:examples/target/accumulo-access-examples-1.0.0-SNAPSHOT.jar
--module=accumulo.access.examples/org.apache.accumulo.access.examples.AccessExample
RED BLUE
+java
--module-path=modules/core/target/accumulo-access-core-1.0.0-SNAPSHOT.jar:modules/examples/target/accumulo-access-examples-1.0.0-SNAPSHOT.jar
--module=org.apache.accumulo.access.examples/org.apache.accumulo.access.examples.AccessExample
Review Comment:
Have you tried running these commands to see if they still work?
##########
modules/core/src/test/java/org/apache/accumulo/access/tests/AccessEvaluatorTest.java:
##########
@@ -156,9 +155,8 @@ private static void runTestCases(Access accumuloAccess,
TestDataSet testSet,
() -> accumuloAccess.newExpression(expression), expression);
assertThrows(InvalidAccessExpressionException.class,
() -> accumuloAccess.newParsedExpression(expression),
expression);
- break;
- default:
- throw new IllegalArgumentException();
+ }
+ default -> throw new IllegalArgumentException();
Review Comment:
May not need this.
```suggestion
```
##########
modules/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java:
##########
@@ -77,7 +77,7 @@ public static CharSequence quote(CharSequence term) {
}
public static CharSequence unquote(CharSequence term) {
- if (term.equals("\"\"") || term.isEmpty()) {
+ if (term.toString().equals("\"\"") || term.isEmpty()) {
Review Comment:
The `toString()` call could be expensive, sometimes the code passes in a
CharSeq backed by an array and toString() will copy and allocate. Could rework
the code to do the following to avoid the toString, also avoid checking for
quotes twice.
```java
public static CharSequence unquote(CharSequence term) {
if (term.length() >=2 && term.charAt(0) == '"' &&
term.charAt(term.length() - 1) == '"') {
term = AccessEvaluatorImpl.unescape(term.subSequence(1,
term.length() - 1));
}
if(term.isEmpty()){
throw new IllegalArgumentException("Empty strings are not legal
authorizations.");
}
return term;
}
```
--
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]
