Skip to content

Comments

fix: format and clean code in modules#2439

Merged
imbajin merged 13 commits intoapache:masterfrom
msgui:format-
Feb 20, 2024
Merged

fix: format and clean code in modules#2439
imbajin merged 13 commits intoapache:masterfrom
msgui:format-

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
@codecov
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

}

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

- 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)

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
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 <jin@apache.org>
@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