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

Remove IActionContext.NewGuid() & Add RandomExtension.GenerateRandomGuid() #508

Merged
merged 4 commits into from
Sep 10, 2019

Conversation

Unengine
Copy link
Contributor

@Unengine Unengine commented Sep 10, 2019

I think getting a random Guid in IActionContext is a bad usage because dummy objects (for testing, debugging or etc ...) using random Guid in games don't need IActionContext. IRandoms are enough to acquire the same thing.

So I removed IActionContext.NewGuid().
Instead I added an extension class, RandomExtension that includes a extension method, GenerateRandomGuid().
Since RandomExtension is an extension class, I implemented generating a random Guid.

@CLAassistant
Copy link

CLAassistant commented Sep 10, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@longfin longfin left a comment

Choose a reason for hiding this comment

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

Is it okay without removing ActionContext.NewGuid()?

/// <seealso cref="IRandom"/>
public static Guid GenerateRandomGuid(this IRandom random)
{
var bytes = new byte[16];
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if there is mention as this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are fixed! Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Unengine Still not addressed I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to commit. I'll also commit your suggestions at once!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Job's done!

Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

Please write two entries to the changelog file.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

Merging #508 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.02%     
==========================================
  Files         200      201       +1     
  Lines       14718    14718              
==========================================
+ Hits        13254    13257       +3     
+ Misses       1180     1177       -3     
  Partials      284      284
Impacted Files Coverage Δ
Libplanet/Action/ActionContext.cs 100% <ø> (ø) ⬆️
Libplanet/Action/RandomExtension.cs 100% <100%> (ø)
Libplanet.Tests/Action/ActionContextTest.cs 89.7% <100%> (ø) ⬆️
Libplanet/Net/Protocols/KademliaProtocol.cs 60.86% <0%> (+0.86%) ⬆️

limebell
limebell previously approved these changes Sep 10, 2019
Copy link
Member

@limebell limebell left a comment

Choose a reason for hiding this comment

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

I think you should fix some tests which uses NewGuid().

@limebell limebell self-requested a review September 10, 2019 10:17
@Unengine
Copy link
Contributor Author

I think you should fix some tests which uses NewGuid().

You're right. Job's done!

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@longfin longfin left a comment

Choose a reason for hiding this comment

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

🎉

@dahlia dahlia merged commit 738bb57 into planetarium:master Sep 10, 2019
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