-
Notifications
You must be signed in to change notification settings - Fork 48
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 duplicate collection id #2508
Conversation
ipdae
commented
Apr 3, 2024
- resolve 콜렉션 스탯 적용때 중복값 제거하기 #2501
- distinct collection ids when serialize & deserialize collection state
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Lib9c/Model/State/CollectionState.cs
Outdated
@@ -22,12 +25,25 @@ public CollectionState(List serialized) | |||
{ | |||
Ids.Add((Integer)value); | |||
} | |||
|
|||
Ids = Ids.Distinct().ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ids를 Set과 같은 형태로 관리해도 되지 않을까요?
Lib9c/Model/State/CollectionState.cs
Outdated
} | ||
|
||
public CollectionState(IValue bencoded) : this((List)bencoded) | ||
{ | ||
} | ||
|
||
public IValue Bencoded => List.Empty.Add(new List(Ids)); | ||
public IValue Bencoded => List.Empty.Add(new List(Ids.Distinct())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 Distinct 한 뒤에 List 내의 순서 같은게 달라서 상태 불일치가 생길 가능성은 없을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직렬화된 상태가 순서에 영향을 안받는것으로 알고 있습니다. 정렬이 필요할까요? @planetarium/libplanet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 그럼 이 내용은 엔진팀에서 확인&답변 부탁드립니다🙇♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Distinct
자체가 정렬을 담보하지는 않지만, List<T>.Distinct
자체가 원래 있던 순서를 유지하는 동작을 하고 있으므로 상관은 없을 겁니다.
다만 @sonohoshi 님이 말씀하신 대로 그렇게 좋은 프랙티스는 아니라고 생각하고, Active Migration 이후에 코드를 지우는 일은 필요 해 보이긴 합니다.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
SortedSet을 사용하도록 변경했습니다. 다시 한번 리뷰 부탁드립니다. @sonohoshi @eugene-doobu |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |