-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor AvatarState
constructor
#2780
Conversation
cef3a16
to
3330248
Compare
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.
What is the scope of the update changes for relate repositories(NineChronicles, etc) that use AvatarState?
address, agentAddress, blockIndex, questListVar, worldInformationVar, rankingMapAddress, name); | ||
} | ||
|
||
public AvatarState(Address address, |
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.
What if we take the constructor private and depend only on "AvatarState.Create"?
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.
The purpose of this pull request was to easily create an AvatarState
instance without using AvatarSheets
. We can change the constructor's access modifier to internal
and allow to access to internal
elements to the Lib9c.Tests
project (it already opened), but it cannot open to NIneChronicles.Headless.Tests
and other external test projects.
When I searched |
Description
This pull request only adjusts the constructor of
AvatarState
and introduces a new static factory method,AvatarState.Create
. With some huge changes (almost from test code) by replacing theAvatarState
constructor with the factory method.Before this pull request, it was hard to create an
AvatarState
instance because its constructor was heavily dependent onAvatarSheets
. Even if I want to create a simple customAvatarState
instance, it requiresAvatarSheets
(CSV data).So it will become to provide a simple constructor (just setting properties and fields) and handle something more with the factory method.
Reviewers
I requested reviews for @ipdae, @sonohoshi, @eugene-doobu who are actively working on Lib9c. If you know someone else who would be good for this pull request, please nominate them.
Future work plan
If this pull request is accepted, I'll refactor
WorldInformation
andQuestList
too if I need. These works will be helpful for #2685 and planetarium/NineChronicles.Headless#2540.