Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: format and clean code in modules #2439

Merged
merged 13 commits into from
Feb 20, 2024
Merged

fix: format and clean code in modules #2439

merged 13 commits into from
Feb 20, 2024

Conversation

msgui
Copy link
Contributor

@msgui msgui commented Feb 2, 2024

Purpose of the PR

Main Changes

Format & clean code in submodels:

  1. API
  2. Scylladb
  3. Postgresql
  4. Rocksdb
  5. Palo
  6. Mysql
  7. Hbase
  8. Cassandra
  9. Test

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. api Changes of API cassandra Cassandra backend rocksdb RocksDB backend labels Feb 2, 2024
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 79 lines in your changes are missing coverage. Please review.

Comparison is base (7586779) 66.21% compared to head (e13dea5) 66.23%.

Files Patch % Lines
...n/java/org/apache/hugegraph/api/graph/EdgeAPI.java 58.62% 7 Missing and 5 partials ⚠️
...java/org/apache/hugegraph/api/graph/VertexAPI.java 9.09% 9 Missing and 1 partial ⚠️
.../org/apache/hugegraph/opencypher/CypherPlugin.java 0.00% 8 Missing ⚠️
...egraph/backend/store/cassandra/CassandraShard.java 37.50% 5 Missing ⚠️
...egraph/backend/store/cassandra/CassandraStore.java 50.00% 4 Missing and 1 partial ⚠️
.../java/org/apache/hugegraph/api/graph/BatchAPI.java 33.33% 4 Missing ⚠️
.../apache/hugegraph/api/filter/LoadDetectFilter.java 25.00% 3 Missing ⚠️
...ain/java/org/apache/hugegraph/api/job/TaskAPI.java 0.00% 3 Missing ⚠️
...egraph/backend/store/cassandra/CassandraTable.java 62.50% 1 Missing and 2 partials ⚠️
...raph/backend/store/rocksdb/RocksDBStdSessions.java 81.25% 3 Missing ⚠️
... and 16 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2439      +/-   ##
============================================
+ Coverage     66.21%   66.23%   +0.01%     
+ Complexity      828      827       -1     
============================================
  Files           511      511              
  Lines         42598    42594       -4     
  Branches       5942     5938       -4     
============================================
+ Hits          28208    28212       +4     
+ Misses        11582    11576       -6     
+ Partials       2808     2806       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msgui msgui changed the title chore: Format&Clean code fix: Format and clean code in modules Feb 2, 2024
@dosubot dosubot bot modified the milestones: 1.3.0, 1.5.0 Feb 2, 2024
@msgui msgui changed the title fix: Format and clean code in modules fix: format and clean code in modules Feb 2, 2024
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Feb 7, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. size:L This PR changes 100-499 lines, ignoring generated files. labels Feb 7, 2024
@msgui msgui requested a review from imbajin February 7, 2024 06:06
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

A lot of align could be merged into one line (manually), but it will cost some time

Comment on lines 162 to 165
protected String escapeAndWrapString(String value) {
if (value.equals("\u0000")) {
return "\'\'";
return "''";
Copy link
Member

Choose a reason for hiding this comment

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

better add a test or test it in local env first? (at least maybe add a PR comment here)

Comment on lines 151 to 154
final StringBuilder builder = new StringBuilder("CypherClient{");
builder.append("userName='").append(userName).append('\'')
.append(", token='").append(token).append('\'').append('}');
String builder = "CypherClient{" + "userName='" + userName + '\'' +
", token='" + token + '\'' + '}';

return builder.toString();
return builder;
Copy link
Member

Choose a reason for hiding this comment

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

ignore the clean for StringBuilder (due to perf influence, check if we could config the rule to ignore it)

we could remove the unless SB transfer if we could ensure it's the call frequency is low

Context context = getContext();
if (context == null) {
return null;
}
return context.user().toJson();
}

protected static void logUser(User user, String path) {
static void logUser(User user, String path) {
Copy link
Member

Choose a reason for hiding this comment

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

consider keeping them first (modification should be separated & careful)

Copy link
Member

Choose a reason for hiding this comment

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

ensure no UI diff
image

@@ -49,7 +49,7 @@ services:
networks:
- ca-network
healthcheck:
test: ["CMD", "cqlsh", "--execute", "describe keyspaces;"]
test: [ "CMD", "cqlsh", "--execute", "describe keyspaces;" ]
Copy link
Member

Choose a reason for hiding this comment

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

seems better to config the yaml file rules to keep it as the origin style?

Comment on lines 18 to 19
:remote connect tinkerpop.server conf/remote.yaml
:> hugegraph
: remote connect tinkerpop . server conf / remote.yaml
: > hugegraph
Copy link
Member

Choose a reason for hiding this comment

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

need ignore/exclude it

Comment on lines 35 to 36
org.apache.hugegraph.plugin.HugeGraphGremlinPlugin: { },
org.apache.tinkerpop.gremlin.server.jsr223.GremlinServerGremlinPlugin: { },
Copy link
Member

Choose a reason for hiding this comment

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

keep original style

Comment on lines 4121 to 4123
final double min15 = new BigDecimal(1.234567890987654321d)
.setScale(15, BigDecimal.ROUND_DOWN)
final double min15 = new BigDecimal("1.234567890987654321")
.setScale(15, RoundingMode.DOWN)
Copy link
Member

Choose a reason for hiding this comment

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

double & string number are the same case? need check it carefully

Comment on lines 7610 to 7611
long splitSize = 1 * 1024 * 1024 - 1;
long splitSize = 1024 * 1024 - 1;
Copy link
Member

Choose a reason for hiding this comment

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

better keep it if we could config the rule (if we can't just remove it)

@@ -9140,7 +9141,7 @@ public void testEnhanceTextMatch() {
Assert.assertEquals(1, vertices.size());
Assert.assertTrue(vertices.contains(vertex5));

vertices = g.V().has("name", Text.contains("(秦始皇2|秦始皇3)")).toList();
vertices = g.V().has("name", Text.contains("(秦始皇 2|秦始皇 3)")).toList();
Copy link
Member

Choose a reason for hiding this comment

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

be careful to change the space in code? (better just changed them in comment) to avoid risks

@msgui msgui requested a review from imbajin February 19, 2024 10:04
TODO: address the align space (now we use 8 instead 4?)
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

Most of the files were checked except for some clean rules that did not take effect (consider address them together in another PR )

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 19, 2024
@imbajin imbajin requested a review from VGalaxies February 19, 2024 12:30
@imbajin imbajin merged commit 5cb9aad into apache:master Feb 20, 2024
21 checks passed
VGalaxies pushed a commit that referenced this pull request Feb 20, 2024
Format & clean code in submodels:

1. API
2. Scylladb
3. Postgresql
4. Rocksdb
5. Palo
6. Mysql
7. Hbase
8. Cassandra
9. Test

---------

Co-authored-by: imbajin <[email protected]>
@msgui msgui deleted the format- branch March 15, 2024 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Changes of API cassandra Cassandra backend lgtm This PR has been approved by a maintainer rocksdb RocksDB backend size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants