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

Chore: Change "?" for "Option" for optional fields #480

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

lmuntaner
Copy link
Contributor

@lmuntaner lmuntaner commented Nov 22, 2023

Motivation

Using ? is not the same as Option. With ? the field is optional, but with Option, the field is mandatory, but the value can be undefined.

This small distinction is important when we render the information of proposals in NNS Dapp. We convert the actions to camelcase and remove [] | [T].

If we use ?, we might forget to convert it and it will never show up in NNS Dapp. By using Option, we make sure that the field will always we present in the converters.

You can see the example in the params, maxDirectParticipationIcpE8s and minDirectParticipationIcpE8s fields for the (deprecated) OpenSnsTokenSwap proposal action that I had to change when I moved from ? to Option.

Changes

  • Change the ? in types/governance_converters.ts for Option. Only for the fields related to proposals.

Tests

  • Fix a converter after changes.

Todos

  • Add entry to changelog (if necessary).

@lmuntaner
Copy link
Contributor Author

@mstrasinskis @bitdivine please review

@bitdivine this was the only thing I saw we can do better to catch possible errors.

The process for these fields is rather automatic, put it to camel case and extract the value from [] | [T]. But I don't think there is much value in automating this part. It can be prone to errors and for no much time saved.

Copy link
Contributor

github-actions bot commented Nov 22, 2023

size-limit report 📦

Path Size
@dfinity/ckbtc 6.86 KB (0%)
@dfinity/cmc 1.01 KB (0%)
@dfinity/ledger-icrc 2.92 KB (0%)
@dfinity/ledger-icp 14.41 KB (0%)
@dfinity/nns 33.83 KB (+0.05% 🔺)
@dfinity/nns-proto 76.3 KB (0%)
@dfinity/sns 14.92 KB (0%)
@dfinity/utils 4.12 KB (0%)
@dfinity/ic-management 1.94 KB (0%)

@lmuntaner lmuntaner added the Planned Work planned in the Sprint label Nov 22, 2023 — with Graphite App
Copy link
Contributor

@mstrasinskis mstrasinskis left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@lmuntaner lmuntaner merged commit 63d29e5 into main Nov 22, 2023
12 checks passed
@lmuntaner lmuntaner deleted the chore_LM_change-optional-field-for-Option-helper branch November 22, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Planned Work planned in the Sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants