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

Extract PolymorphicAction<T> from Transaction<T> & IAction #169

Merged
merged 9 commits into from Apr 3, 2019

Conversation

Projects
None yet
3 participants
@dahlia
Copy link
Member

commented Apr 2, 2019

This patch extracts the subtype dispatcher which had been coupled to Transaction<T> and IAction into a new class PolymorphicAction<T>. This is both a concrete IAction implementation and a decorator of an abstract action class having subclasses. This also is an alternative approach to make Libplanet.Explorer standalone (see also #152) besides @earlbread's PR #161.

Please read added changelogs and XML comments on IAction and PolymorphicAction<T>for details.

Additionally, I renamed InvalidActionTypeExtension (introduced by #144) to MissingActionTypeException.

@dahlia dahlia self-assigned this Apr 2, 2019

@dahlia dahlia requested review from longfin and earlbread Apr 2, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 2, 2019

Codecov Report

Merging #169 into master will decrease coverage by 0.05%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
- Coverage   84.17%   84.11%   -0.06%     
==========================================
  Files          69       70       +1     
  Lines        3165     3185      +20     
==========================================
+ Hits         2664     2679      +15     
- Misses        501      506       +5
Impacted Files Coverage Δ
Libplanet/Store/TransactionSet.cs 96.77% <ø> (ø) ⬆️
Libplanet/Blocks/Block.cs 97.95% <ø> (ø) ⬆️
Libplanet/Blockchain/Policies/BlockPolicy.cs 100% <ø> (ø) ⬆️
Libplanet/Blockchain/BlockChain.cs 98.37% <ø> (ø) ⬆️
Libplanet/Net/Swarm.cs 82.49% <ø> (-1.26%) ⬇️
Libplanet/Store/BaseStore.cs 100% <ø> (ø) ⬆️
Libplanet/Store/FileStore.cs 93.97% <ø> (ø) ⬆️
Libplanet/Store/BlockSet.cs 96.66% <ø> (ø) ⬆️
Libplanet/Tx/Transaction.cs 94.83% <100%> (-0.1%) ⬇️
Libplanet/Action/PolymorphicAction.cs 100% <100%> (ø)
... and 5 more

@dahlia dahlia force-pushed the dahlia:polymorphic-action branch from a0a901a to e7df1a6 Apr 3, 2019

Show resolved Hide resolved CHANGES.md Outdated
{ "values", a.PlainValue },
})
actions: Actions.Select(a =>
a.PlainValue.ToDictionary(

This comment has been minimized.

Copy link
@longfin

longfin Apr 3, 2019

Member

Maybe it's better to change the type of RawTransaction.Actions... 😕

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Author Member

Currently RawTransaction consists of many mutable types like byte[] and Dictionary<K, V>. If we want RawTransaction to be freer from state I believe we need to adjust other fields too. Seems like it should be a separated patch.

Show resolved Hide resolved Libplanet.Tests/Common/Action/DumbAction.cs Outdated
Show resolved Hide resolved CHANGES.md Outdated
var values = (IDictionary<string, object>)arg["values"];
action.LoadPlainValue(values.ToImmutableDictionary());
var action = new T();
action.LoadPlainValue(arg.ToImmutableDictionary());

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 3, 2019

Member

Can we deserialize the actions properly without concrete action type?

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Author Member

Did you mean, what if T is not a concrete type?

This comment has been minimized.

Copy link
@earlbread

earlbread Apr 3, 2019

Member

Right, what if we use like DumbAction?

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 3, 2019

Author Member

That's fine because DumbAction is a concrete class. According to the official docs, new() constraint is satisfied by only concrete class, neither abstract class nor interface:

To use the new constraint, the type cannot be abstract.

That means, from now on we only accept a concrete class as an action type — even PolymorphicAction<T> in itself is concrete. This is a point of getting rid of built-in subtype dispatcher. Note that T of PolymorphicAction<T> does not have new() constraint on the other hand. It can take an abstract class or an interface.

@longfin

longfin approved these changes Apr 3, 2019

@dahlia

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I adjusted the code according @longfin's review comments, wrote more tests, and added implicit operator PolymorphicAction<T>(T) as well for convenience.

@longfin @earlbread Please review this again.

@dahlia dahlia merged commit 8abbb26 into planetarium:master Apr 3, 2019

5 checks passed

Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/patch 97.61% of diff hit (target 84.17%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +13.44% compared to 40a6476
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.