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

Ftr: add collection support according to java implementation #161

Merged
merged 6 commits into from
Mar 15, 2020

Conversation

YGrylls
Copy link
Contributor

@YGrylls YGrylls commented Mar 6, 2020

What this PR does:
Add support for java collections such as HashSet
Collections are encoded through Serializer and decoded as list with regard of its type (collection types are encoded as the type of FixedTypedList in java impl of hessian).
The collection struct implements are open for users, see what java_collection_test.go does.
Issue #122

Special notes for your reviewer:
To reach the feature, code of method "readTypedList" in list.go also changes a little.
Related test codes of java side have also been added, see the last methods in TestCustomDecode.java & TestCustomReply.java

Does this PR introduce a user-facing change?:

Users need to implements collection structs and set them into go-hessian2, examples can be found in java_collection_test.go

@AlexStocks
Copy link
Contributor

pls update readme about this feature.

@AlexStocks
Copy link
Contributor

@hxmhlt @fangyincheng pls own this pr.

return "java.util.HashSet"
}

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

top init()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

SetCollectionSerialize(&JavaHashSet{})
}

type JavaHashSet struct {
Copy link
Member

Choose a reason for hiding this comment

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

在gost中我们已经实现了正在的hashset,这个是否可以复用?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这边设计设想是依赖接口,集合的具体实现开放给用户或外部来实现;这里只是一个实现了接口的简单例子

Copy link
Contributor

Choose a reason for hiding this comment

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

我到不觉得是例子,一旦加入实际上则成为了标准,用户也不会另外实现。 所以我也建议尝试复用gost里面的hashset。

@codecov-io
Copy link

codecov-io commented Mar 14, 2020

Codecov Report

Merging #161 into master will decrease coverage by 0.28%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
- Coverage   66.45%   66.17%   -0.29%     
==========================================
  Files          21       22       +1     
  Lines        2573     2652      +79     
==========================================
+ Hits         1710     1755      +45     
- Misses        667      691      +24     
- Partials      196      206      +10     
Impacted Files Coverage Δ
java_collection.go 54.66% <54.66%> (ø)
list.go 85.38% <100.00%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1d1660...c1d9039. Read the comment docs.

java_collection.go Outdated Show resolved Hide resolved
java_collection.go Outdated Show resolved Hide resolved
@AlexStocks
Copy link
Contributor

LGTM

Copy link
Contributor

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks AlexStocks merged commit 9e2e48b into apache:master Mar 15, 2020
zhaoyunxing92 pushed a commit that referenced this pull request Sep 4, 2021
Ftr: add collection support according to java implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants