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

docs: Add inline comments to pallet grouping macro expansions #47

Merged
merged 10 commits into from
Dec 13, 2024

Conversation

mittal-parth
Copy link
Contributor

Fixes #24

Adds inline comments to the various pallet grouping macro expansions.

PR Checklist

  • Documentation

@mittal-parth mittal-parth changed the title assets: Add inline comments docs: Add inline comments to pallet grouping macro expansions Nov 14, 2024
src/assets.rs Outdated Show resolved Hide resolved
src/assets.rs Outdated Show resolved Hide resolved
@mittal-parth
Copy link
Contributor Author

Hi @4meta5!

I have pushed changes for all groups with some exceptions for which I couldn't find any relevant docs. Would be great if you could help with those.

Assets

  • pallet_asset_manager

EVM

  • pallet_ethereum
  • pallet_base_fee
  • pallet_erc20_xcm_bridge

Tanssi

  • pallet_author_inherent
  • pallet_cc_authorities_noting

XCM

  • orml_xtokens
  • pallet_xcm_transactor

@mittal-parth mittal-parth marked this pull request as ready for review November 27, 2024 17:56
@mittal-parth
Copy link
Contributor Author

@4meta5 lets close this?
Thanks!

src/assets.rs Outdated Show resolved Hide resolved
src/evm.rs Outdated Show resolved Hide resolved
src/xcm.rs Outdated Show resolved Hide resolved
@mittal-parth
Copy link
Contributor Author

@4meta5 thanks for the review.

What to do for the pallets mentioned in the above comment?

@4meta5
Copy link
Collaborator

4meta5 commented Dec 3, 2024

Created this follow up issue: #55 for the pallets not covered by this PR. Thanks for your contribution!

@mittal-parth
Copy link
Contributor Author

The testing error is because there are no tests to run which have become an error now (was a warning previously).

Test failure now:

Nextest run ID 1bc14f77-1f2f-4cf4-8cb8-9ba87c6ed9d7 with nextest profile: default
   Starting 0 tests across 1 binary
------------
    Summary [   0.000s] 0 tests run: 0 passed, 0 skipped
error: no tests to run
(hint: use `--no-tests` to customize)

Old behaviour in the last successful PR merged

 Nextest run ID c3c96e9a-e0e9-453a-a5f8-59adb16d5605 with nextest profile: default
    Starting 0 tests across 1 binary
------------
     Summary [   0.000s] 0 tests run: 0 passed, 0 skipped
warning: no tests to run -- this will become an error in the future
(hint: use `--no-tests` to customize)

We could either pass the --no-tests flag or add a sanity check test.

@4meta5
Copy link
Collaborator

4meta5 commented Dec 4, 2024

I experienced this as well in #41 and added a sanity test in this commit 074f7c3

Feel free to do the same in your PR to make the CI green or you can wait for #41 to be merged and then merge into main

@mittal-parth
Copy link
Contributor Author

@4meta5 done

@mittal-parth
Copy link
Contributor Author

@4meta5 Lets wrap this up I suppose, the CI should be green now

Thanks!

@mittal-parth mittal-parth force-pushed the pallet-macros-comments branch from de51780 to 0792f71 Compare December 12, 2024 03:33
@mittal-parth
Copy link
Contributor Author

Rebased off main, resolved conflictts

src/assets.rs Outdated Show resolved Hide resolved
src/assets.rs Outdated Show resolved Hide resolved
src/assets.rs Outdated Show resolved Hide resolved
@mittal-parth
Copy link
Contributor Author

On it

@4meta5
Copy link
Collaborator

4meta5 commented Dec 12, 2024

@KitHat @ggonzalez94 please lend a 2nd pair of eyes to confirm that no code configs have been changed, I just did another quick skim but it is a lot of LOC changed :)

@4meta5 4meta5 requested review from KitHat and ggonzalez94 December 12, 2024 16:58
@mittal-parth
Copy link
Contributor Author

mittal-parth commented Dec 12, 2024

Pushed changes

Apologies for so much back and forth 🤧

src/assets.rs Outdated Show resolved Hide resolved
src/assets.rs Outdated Show resolved Hide resolved
@mittal-parth mittal-parth force-pushed the pallet-macros-comments branch from dd27432 to 5aa1c2c Compare December 12, 2024 19:54
@4meta5 4meta5 merged commit 6706a59 into OpenZeppelin:main Dec 13, 2024
4 checks passed
@4meta5
Copy link
Collaborator

4meta5 commented Dec 13, 2024

Thank you for your perseverance @mittal-parth

@mittal-parth
Copy link
Contributor Author

Thanks for your patience too! @4meta5

Lemme know if I can help with something else :)

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.

Clean code comments in macro expansion (from templates)
2 participants