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: impl Storable for Principal #128

Closed
wants to merge 1 commit into from

Conversation

witter-deland
Copy link
Contributor

No description provided.

@ghost ghost added the cla:agreed label Sep 13, 2023
@roman-kashitsyn
Copy link
Contributor

Note that candid is a dev dependency, not the library dependency, so CI fails with the following error:

 --> src/storable.rs:6:5
  |
6 | use candid::Principal;
  |     ^^^^^^ use of undeclared crate or module `candid`

Given how hard it is to bump Candid versions, we'd prefer not to add candid as a dependency to add a few impls.

@witter-deland
Copy link
Contributor Author

Got it.

Note that candid is a dev dependency, not the library dependency, so CI fails with the following error:

 --> src/storable.rs:6:5
  |
6 | use candid::Principal;
  |     ^^^^^^ use of undeclared crate or module `candid`

Given how hard it is to bump Candid versions, we'd prefer not to add candid as a dependency to add a few impls.

@witter-deland
Copy link
Contributor Author

One personal opinion is: the types in candid are unavoidable in canister development, especially when stored in a stable structure. Supporting these types will be more friendly to developers.

@roman-kashitsyn
Copy link
Contributor

the types in candid are unavoidable in canister development

I agree, but we don't want to force the specific Candid version on the users. We went through a very painful 0.8 -> 0.9 candid upgrade recently; the fewer libraries fix a specific candid version, the easier it's to coordinate version bumps.

@witter-deland
Copy link
Contributor Author

I agree, but we don't want to force the specific Candid version on the users. We went through a very painful 0.8 -> 0.9 candid upgrade recently; the fewer libraries fix a specific candid version, the easier it's to coordinate version bumps.

Got it, thank you for your reply, this PR can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants