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: helpers and actions #2

Merged
merged 1 commit into from
Mar 18, 2025
Merged

feat: helpers and actions #2

merged 1 commit into from
Mar 18, 2025

Conversation

deluca-mike
Copy link
Collaborator

No description provided.

@deluca-mike deluca-mike added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 17, 2025
@deluca-mike deluca-mike requested review from neekolas and fbac March 17, 2025 18:05
@deluca-mike deluca-mike self-assigned this Mar 17, 2025
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@deluca-mike deluca-mike force-pushed the feat/helpers branch 6 times, most recently from 19e3cd2 to 607a54a Compare March 18, 2025 02:00
fbac
fbac previously approved these changes Mar 18, 2025
@fbac
Copy link
Collaborator

fbac commented Mar 18, 2025

Are we losing soldeer with this pr?

@deluca-mike deluca-mike force-pushed the feat/helpers branch 3 times, most recently from 2a33db9 to 54411b1 Compare March 18, 2025 06:59
@deluca-mike
Copy link
Collaborator Author

deluca-mike commented Mar 18, 2025

Are we losing soldeer with this pr?

Sadly yes, for several reasons (listed in most significant to least)

  • it seems to force the names of the dependencies to have their version numbers in the directory name, which means upgrading minor or patch versions or dependencies breaks code without going through all files to rename all imports
  • it adds nothing that git submodules don't already do, but with less flexibility, and less portability (repos that use this repo as a dependency will need to use remapings or soldeer itself)
  • it creates remappings that (at least for me) don't play well with "Go To Definition" features (without more manual intervention)
  • it causes a .git issue (that must be click-ignored) with Fork (a git UI) every time a commit/branch is checked out

I always wanted to give soldeer a try, having come from using git submodules for years, but after playing around with it for a week, it doesn't seem to offer any real benefits.

I have found that relative paths in imports result in the most portable and "fix free" codebase. I was hoping soldeer would be the best of both worlds.

But if we can address some of the above, would be happy to use soldeer. It is possible I am using it wrong.

@xmtp xmtp deleted a comment from github-actions bot Mar 18, 2025
Copy link

LCOV of commit a34a18c during Solidity #12

Summary coverage rate:
  lines......: 98.0% (245 of 250 lines)
  functions..: 96.9% (63 of 65 functions)
  branches...: 92.3% (48 of 52 branches)

Files changed coverage rate:
                         |Lines       |Functions  |Branches    
  Filename               |Rate     Num|Rate    Num|Rate     Num
  =============================================================
  src/GroupMessages.sol  |25.6%     39|70.0%    10|    -      0
  src/IdentityUpdates.sol|25.6%     39|70.0%    10|    -      0
  src/Nodes.sol          |27.5%    131|91.2%    34|    -      0
  src/RatesManager.sol   |25.0%     36|77.8%     9|    -      0

@fbac fbac self-requested a review March 18, 2025 14:53
@fbac
Copy link
Collaborator

fbac commented Mar 18, 2025

Are we losing soldeer with this pr?

Sadly yes, for several reasons (listed in most significant to least)

  • it seems to force the names of the dependencies to have their version numbers in the directory name, which means upgrading minor or patch versions or dependencies breaks code without going through all files to rename all imports

I'm not sure I follow you. It doesn't do that and you can reference dependencies as "@openzeppelin-contracts-upgradeable/access/AccessControlUpgradeable.sol";

  • it creates remappings that (at least for me) don't play well with "Go To Definition" features (without more manual intervention)

Also don't follow, this seems to be a local misconfiguration.

  • it causes a .git issue (that must be click-ignored) with Fork (a git UI) every time a commit/branch is checked out

Also local/subjective.

@deluca-mike deluca-mike merged commit 6f2de5e into main Mar 18, 2025
9 checks passed
@deluca-mike deluca-mike deleted the feat/helpers branch March 18, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants