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

Added possibility to not sort fields #209

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Sep 3, 2023

This PR aims at allowing projects to not sort their ExecuteMsg and QueryMsg field when using the ExecuteFns and QueryFns macros.

Introducing a new macro attribute disable_fields_sorting .
The fields are still sorted by default

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 3, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: b644659
Status:⚡️  Build in progress...

View logs

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #209 (0621450) into main (1cb34c6) will increase coverage by 0.7%.
Report is 2 commits behind head on main.
The diff coverage is 96.0%.

❗ Current head 0621450 differs from pull request most recent head b644659. Consider uploading reports for the commit b644659 to get more accurate results

Files Changed Coverage Δ
contracts/mock_contract/src/lib.rs 94.0% <ø> (ø)
packages/cw-orch-fns-derive/src/lib.rs 100.0% <ø> (ø)
contracts/mock_contract/src/msg_tests.rs 94.8% <94.8%> (ø)
packages/cw-orch-fns-derive/src/execute_fns.rs 99.1% <100.0%> (+<0.1%) ⬆️
packages/cw-orch-fns-derive/src/helpers.rs 66.0% <100.0%> (+20.5%) ⬆️
packages/cw-orch-fns-derive/src/query_fns.rs 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing it! Would love for that feature to be ON by default, but it's probably too late 😢

contracts/mock_contract/src/msg_tests.rs Show resolved Hide resolved
packages/cw-orch-fns-derive/src/helpers.rs Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@CyberHoward
Copy link
Contributor

CyberHoward commented Sep 11, 2023

Thanks for implementing it! Would love for that feature to be ON by default, but it's probably too late 😢

Why would you want it on by default? Imo it's still dangerous...
We have well-tested code but that doesn't mean all our users will have.

@Kayanski Kayanski merged commit 9203f33 into main Sep 11, 2023
15 of 16 checks passed
@Kayanski Kayanski deleted the nicolas/orc-22-allow-disabling-sorting-of-method-arguments-generated-by branch September 11, 2023 11:11
@Buckram123
Copy link
Contributor

Why would you want it on by default? Imo it's still dangerous... We have well-tested code but that doesn't mean all our users will have.

IMO sorting it is as dangerous as not sorting them:

  • You don't sort - field swap can screw you
  • You sort - field rename can screw you

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