Skip to content

Conversation

@florian-lefebvre
Copy link
Member

@changeset-bot
Copy link

changeset-bot bot commented Nov 19, 2025

⚠️ No Changeset found

Latest commit: 752c6f7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Here's my honest feedback:

  • the coding example isn't concrete. You mixed different code, which makes it very difficult to understand how everything is mixed together. We start with the key and logger, but then there's no logger anymore, we eventually end up showing some helpers. This doesn't help understanding how the whole code fits together
  • we start with business logic and infrastructure, but then I don't know where the business logic is anymore
  • we don't show how the final usage would look like
  • we don't show what the final testing would look like, and what we should test. We mentioned only what we shouldn't
  • overall there are many assumptions, and even after reading this multiple times, I still can't understand why some interfaces are in domain.ts, while others are in definitions/
  • overall, don't be afraid to be a bit more verbose, and since you rely more on the code example, make sure to make it consistent and good

@florian-lefebvre
Copy link
Member Author

Thanks for the feedback! I wasn't sure how detailed it should be, I will make it clearer 👍

@ematipico
Copy link
Member

Thanks for the feedback! I wasn't sure how detailed it should be, I will make it clearer 👍

Well, you're imparting some knowledge to the team and current/future contributors. I believe we should be detailed enough so that even first-time contributors understand how the project works

@florian-lefebvre
Copy link
Member Author

To be clear what I'm proposing here will very likely evolve as we go, this should be considered a living document/guide

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.

3 participants