feat: Framework Init - base template with Outbound transport config #88
Conversation
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
+ Coverage 91.91% 92.39% +0.47%
==========================================
Files 19 23 +4
Lines 668 710 +42
==========================================
+ Hits 614 656 +42
Misses 30 30
Partials 24 24
Continue to review full report at Codecov.
|
rolsonquadras
force-pushed
the
issue-78
branch
from
9b194c7 to
94efeb3
Compare
rolsonquadras
force-pushed
the
issue-78
branch
3 times, most recently
from
e7224c2 to
2265127
Compare
rolsonquadras
force-pushed
the
issue-78
branch
from
2265127 to
cf2367d
Compare
troyronda
requested review from
fqutishat,
sudeshrshetty,
bstasyszyn,
talwinder50 and
baha-ai
rolsonquadras
force-pushed
the
issue-78
branch
2 times, most recently
from
10f3e05 to
f4a9ffc
Compare
rolsonquadras
force-pushed
the
issue-78
branch
from
f4a9ffc to
463122c
Compare
rolsonquadras
force-pushed
the
issue-78
branch
from
463122c to
a31807a
Compare
| require.Error(t, exClient.SendExchangeRequest(req, "")) | ||
| } | ||
|
|
||
| type mockTransportProviderFactory struct { |
There was a problem hiding this comment.
can we move all mocks to common pkg if possible?
There was a problem hiding this comment.
@troyronda @Baha-sk @fqutishat thoughts?
There was a problem hiding this comment.
What is meant by “all mocks”? I generally think mocks should be near its real implementation.
| // New initializes the Aries framework based on the set of options provided. | ||
| func New(opts ...Option) (*Aries, error) { | ||
| // get the default framework options | ||
| defOpts := defFrameworkOpts() |
There was a problem hiding this comment.
Just a thought.
Not sure about having default factory settings as opts, if someone passes custom opts then we may have duplicate opts. It may work here in this case due to order of opts. Need proper test cases to see if overriding actually works (I can see in one of the test you passed mock transport factory), just to be at safe side we shouldn't pass default settings as opts.
Instead at line number 28, initialize Aries struct with default opts.
There was a problem hiding this comment.
This was done based on comments.
There was a problem hiding this comment.
Fine for me if order of opts are intact.
There was a problem hiding this comment.
Yes order of opts needs to be kept intact.
| frameworkOpts := &Aries{} | ||
|
|
||
| // generate framework configs from options | ||
| for _, option := range append(defOpts, opts...) { |
There was a problem hiding this comment.
defOpts has default and append opts to it may duplicate options, this can lead to unexpected behaviour
There was a problem hiding this comment.
The second opts(user opts) will overwrite the defOpts.
There was a problem hiding this comment.
Yes - second opt can override.
The other way around this type of problem seems to be to force framework users to pass the default option, rather than implicitly include as the first option. This has its own negative trade-off, of course.
There was a problem hiding this comment.
Perhaps we could also introduce a functional argument like WithoutDefaults(). Created #110.
rolsonquadras
force-pushed
the
issue-78
branch
from
a31807a to
a26c159
Compare
| frameworkOpts := &Aries{} | ||
|
|
||
| // generate framework configs from options | ||
| for _, option := range append(defOpts, opts...) { |
There was a problem hiding this comment.
Perhaps we could also introduce a functional argument like WithoutDefaults(). Created #110.
- Add New(opts) function which takes in the options to configuration - Support outbound transport configuration Signed-off-by: Rolson Quadras <[email protected]>
rolsonquadras
force-pushed
the
issue-78
branch
from
a26c159 to
2002cc4
Compare
|
BTW: I am wondering if we should have this package directly under pkg (since it is effectively the default entry point for developers). The argument for being under framework is also good - i.e., it is a framework :). Something to think about anyways (#114). |
Closes #78
Signed-off-by: Rolson Quadras [email protected]