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

feat(question-answer): separate logic to a new module #1040

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Oct 2, 2022

Separated the question-answer logic to a new module.

Since it is now a separate module, if core gets released before the question-answer module it will not be included in the core anymore. This is a breaking change and might cause some headaches.

We can just merge it in after 0.3.0 is released and take our time with a strategy to add it back into the core for now.


I could also keep the question-answer module inside the core and release this one separately. When we do a new 0.3.1 or 0.4.0 release the can remove it and make it dependent on this one.

@genaris
Copy link
Contributor

genaris commented Oct 4, 2022

Good to see you are taking your part of the modularisation homework :-)

Just keep in mind that there were some fixes in question-answer, not yet merged into 0.3.0-pre branch so I think are not yet integrated here

@berendsliedrecht
Copy link
Contributor Author

Just keep in mind that there were some fixes in question-answer, not yet merged into 0.3.0-pre branch so I think are not yet integrated here

Good point, will update when 0.3.0-pre is rebased.

@TimoGlastra
Copy link
Contributor

Is there any reason not to merge this before 0.3.0 is released? If it's ready we can merge it right?

@berendsliedrecht
Copy link
Contributor Author

Is there any reason not to merge this before 0.3.0 is released? If it's ready we can merge it right?

It should be possible. However I have a suggestion and I am not sure if it makes sense.

I would like to release this package with the 0.3.0 release and ALSO keep the original module still in core. When we release a 0.3.1 we can take it out. This makes sure that the module is inside core before it is released.

If we release core first it will look at the registry and this module does not exist.

Does this make sense or is there an easier solution?

@TimoGlastra
Copy link
Contributor

Does this make sense or is there an easier solution?

We can't take it out in 0.3.1, as that would be a breaking change and warrant a 0.4.0 release. But I'm also not sure if it makes 100% sense. The packages will be released at the same time, and if you want to use the question answer module you can just add it as as custom module? Why would one be released before the other?

@berendsliedrecht berendsliedrecht marked this pull request as ready for review October 7, 2022 11:58
@berendsliedrecht berendsliedrecht requested a review from a team as a code owner October 7, 2022 11:58
@genaris
Copy link
Contributor

genaris commented Oct 7, 2022

I agree that 0.3.0 is a good timing to do this. The same with Action Menu module. I can do it directly in #1030 or create a new PR like this one once if it's merged (would make the commit history a bit cleaner I guess). Let me know your thoughts!

@berendsliedrecht berendsliedrecht force-pushed the feat/seperate-question-answer branch from c807610 to 464d574 Compare October 7, 2022 12:15
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #1040 (f97203f) into 0.3.0-pre (34658b0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##           0.3.0-pre    #1040      +/-   ##
=============================================
- Coverage      88.24%   88.22%   -0.02%     
=============================================
  Files            680      680              
  Lines          15873    15852      -21     
  Branches        2548     2548              
=============================================
- Hits           14007    13986      -21     
  Misses          1861     1861              
  Partials           5        5              
Impacted Files Coverage Δ
packages/core/src/agent/AgentModules.ts 100.00% <ø> (ø)
packages/core/src/agent/BaseAgent.ts 95.40% <ø> (-0.11%) ⬇️
...ckages/question-answer/src/QuestionAnswerEvents.ts 100.00% <ø> (ø)
packages/question-answer/src/QuestionAnswerRole.ts 100.00% <ø> (ø)
...estion-answer/src/handlers/AnswerMessageHandler.ts 100.00% <ø> (ø)
...tion-answer/src/handlers/QuestionMessageHandler.ts 100.00% <ø> (ø)
packages/question-answer/src/handlers/index.ts 100.00% <ø> (ø)
packages/question-answer/src/index.ts 100.00% <ø> (ø)
packages/question-answer/src/messages/index.ts 100.00% <ø> (ø)
.../question-answer/src/models/QuestionAnswerState.ts 100.00% <ø> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@genaris
Copy link
Contributor

genaris commented Oct 10, 2022

@TimoGlastra @blu3beri, I don't know what you guys think but I'd rather prefer to merge first #1030 and then update this PR with the changes introduced in question-answer module. And in parallel I can extract action-menu so we can get the initial modularisation job done in a clean way before releasing 0.3.0.

@berendsliedrecht
Copy link
Contributor Author

@TimoGlastra @blu3beri, I don't know what you guys think but I'd rather prefer to merge first #1030 and then update this PR with the changes introduced in question-answer module. And in parallel I can extract action-menu so we can get the initial modularisation job done in a clean way before releasing 0.3.0.

Sounds good to me :).

@berendsliedrecht
Copy link
Contributor Author

Thanks for the merge @genaris !

@genaris genaris merged commit 97d3073 into openwallet-foundation:0.3.0-pre Oct 11, 2022
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Oct 13, 2022
…undation#1040)

Signed-off-by: blu3beri <[email protected]>

BREAKING CHANGE: question-answer module has been removed from the core and moved to a separate package. To integrate it in an Agent instance, it can be injected in constructor like this:
```ts
const agent = new Agent({
  config: { /* config */ },
  dependencies: agentDependencies,
  modules: {
    questionAnswer: new QuestionAnswerModule(),
    /* other custom modules */
   }
})
```

Then, module API can be accessed in `agent.modules.questionAnswer`.
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.

4 participants