-
Notifications
You must be signed in to change notification settings - Fork 3
feat: create code snippets for api (#342) #396
base: main
Are you sure you want to change the base?
Conversation
This pull request is being automatically deployed with Vercel (learn more). coderscamp-website – ./🔍 Inspect: https://vercel.com/coderscamp/coderscamp-website/2Uf2LQhPfEbSLzryvNdV6uTss64b coderscamp-storybook – ./🔍 Inspect: https://vercel.com/coderscamp/coderscamp-storybook/A94PiDybNbq16p6aXG8t9HzY1Cca coderscamp-docs – ./🔍 Inspect: https://vercel.com/coderscamp/coderscamp-docs/3wDnrfxBcqpdVGYLHCsDa7jV7CN1 |
Codecov Report
@@ Coverage Diff @@
## main #396 +/- ##
==========================================
+ Coverage 87.87% 87.92% +0.04%
==========================================
Files 150 151 +1
Lines 1806 1830 +24
Branches 236 243 +7
==========================================
+ Hits 1587 1609 +22
- Misses 218 220 +2
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@kacper-cyra Could you write in README or docs how to use it :) ? |
(pastEvents) => {{camelCase domainFunction}}(pastEvents, command), | ||
); |
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.
If you change snippet like I suggested below, then you can change that snippet to this:
(pastEvents) => {{camelCase domainFunction}}(pastEvents, command), | |
); | |
{{camelCase domainFunction}}(command), | |
); |
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.
Maybe, we can go even further and change applicationService.execute
interface to sth like this.
this.applicationService.execute<{{properCase event}}>(
eventStream,
command,
{{camelCase domainFunction}},
);
Then inside execute
we can map command to metadata and we don't need extra currying of domainFunction as proposed by @KrystianKjjk .
What do you think @nowakprojects ?
It will be good to add automationModule generator which add this 4 required files ;) |
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.
Overall it looks good (it's cross-platform and it uses CLI, so it meets our requirements). If I were you, I would add some tests to give some kind of documentation, and what is more important, to protect from bugs if someone would like to change or add a new template. I would recommend you to use snapshot-testing and tmp.dir for that.
(pastEvents) => {{camelCase domainFunction}}(pastEvents, command), | ||
); |
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.
Maybe, we can go even further and change applicationService.execute
interface to sth like this.
this.applicationService.execute<{{properCase event}}>(
eventStream,
command,
{{camelCase domainFunction}},
);
Then inside execute
we can map command to metadata and we don't need extra currying of domainFunction as proposed by @KrystianKjjk .
What do you think @nowakprojects ?
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.
Looks very good for me :)
Big thank you for initiative and implementation.
We can introduce change proposed by @HTK4 in ApplicationService. Of course it could be done in another task.
Snapshot testing for that would be awesome!
Closes #342