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

Add Commons and Toolbox packages #155

Closed
wants to merge 268 commits into from
Closed

Add Commons and Toolbox packages #155

wants to merge 268 commits into from

Conversation

sekimondre
Copy link

Introduces 2 base dependency packages: Commons and Toolbox, and extract some common code to them. They are base dependencies for the SDK and the tests target. We could follow some guidelines for these packages to help maintaining the health of the project on the long term.

Commons:

  • Base dependency for all other packages of the SDK.
  • Must not have any dependency itself.
  • Must not contain code that makes reference to any domain-specific logic (JSON-RPC, storage, engine, etc)
  • Should stay as lean as possible and contain only essential data types and extensions (like AnyCodable).
  • Everything (ideally) should be tested.

Toolbox:

  • Base dependency for test utilities.
  • Should not have any dependencies (unless it really saves code duplication, if we confirm it later).
  • We should avoid domain-specific test doubles (mocks), these can remain in their targets.
  • Should only have code that is used in multiple test targets.

Notes:

  1. Noticing that in WalletConnectUtils there's domain-specific code, it will be split into core components in the future and can be removed when not needed anymore.
  2. Toolbox is a cool name. Commons is still a simple placeholder name, we should try to find a better name to brand the package in the future.
  3. Either type was re-introduced, it will help build a full JSON-RPC compliant API for requests.

@sekimondre sekimondre requested a review from llbartekll April 11, 2022 18:40
@llbartekll
Copy link
Contributor

@MisterVants I think we can create readme for each package and add your guidelines from PR there.

@sekimondre
Copy link
Author

TODO: Test usage of sub-package types with a single import statement

(needs to be done before following up with the new structure)

@sekimondre
Copy link
Author

TODO: Rename TestingUtils to Toolbox and try to reduce dependencies to a single package.

@sekimondre sekimondre marked this pull request as draft April 13, 2022 11:47
@sekimondre
Copy link
Author

TODO: Test usage of sub-package types with a single import statement

(needs to be done before following up with the new structure)

Solution 1: Use typealiases to expose sub-package public types. A list can be maintained in a single file.
Solution 2: Use umbrella frameworks. Might not be the best approach: https://stackoverflow.com/questions/7365578/why-are-umbrella-frameworks-discouraged

Base automatically changed from develop to main May 25, 2022 14:04
@sekimondre
Copy link
Author

Commons package added in PR #256, together with JSONRPC package. TestingUtils to be renamed to Toolbox later.

@sekimondre sekimondre closed this Jun 8, 2022
@sekimondre sekimondre deleted the package-structure branch June 8, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants