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

Add optional serde support #8

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Conversation

SethDusek
Copy link
Collaborator

This will be useful for using BoundedVec in sigma-rust. Also, I was wondering, what are your thoughts on adding a try_push function to BoundedVec? It'd make it a bit easier to use, it could return an Option or Result if there's no more room in the vector.

@coveralls
Copy link

coveralls commented Oct 10, 2021

Pull Request Test Coverage Report for Build 1326189458

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.714%

Totals Coverage Status
Change from base Build 1097957204: 0.0%
Covered Lines: 120
Relevant Lines: 140

💛 - Coveralls

Copy link
Member

@greenhat greenhat left a comment

Choose a reason for hiding this comment

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

Looking great! Thank you for your contribution!
Please check out red CI jobs.

@greenhat
Copy link
Member

This will be useful for using BoundedVec in sigma-rust. Also, I was wondering, what are your thoughts on adding a try_push function to BoundedVec? It'd make it a bit easier to use, it could return an Option or Result if there's no more room in the vector.

Excellent question! It deserves a separate issue - #9

@SethDusek
Copy link
Collaborator Author

Whoops, should be in order now!

@SethDusek
Copy link
Collaborator Author

Hi, not sure what's up with clippy:

Error: Unable to create clippy annotations! Reason: HttpError: Resource not accessible by integration
Warning: It seems that this Action is executed from the forked repository.
Warning: GitHub Actions are not allowed to create Check annotations, when executed for a forked repos. See actions-rs/clippy-check#2 for details.

Running cargo clippy on my system displays no lints.

@greenhat
Copy link
Member

greenhat commented Oct 11, 2021

Hi, not sure what's up with clippy:

Error: Unable to create clippy annotations! Reason: HttpError: Resource not accessible by integration
Warning: It seems that this Action is executed from the forked repository.
Warning: GitHub Actions are not allowed to create Check annotations, when executed for a forked repos. See actions-rs/clippy-check#2 for details.

Running cargo clippy on my system displays no lints.

That's a warning that it's unable to create annotations in PR for clippy warnings. However, I don't understand why clippy sees serde import as unused. I'm merging it. :)

@greenhat greenhat merged commit de77ff0 into ergoplatform:develop Oct 11, 2021
@greenhat
Copy link
Member

@SethDusek Well, it turned out clippy was on to something here. :) With enabled serde feature, build fails on T bounds missing serde traits. I've tried to fix it with no luck. I reverted the changes and made a branch PR for further experiments - #10 with some thoughts on how it could be implemented. Feel free to take over it if you are interested.

@coveralls
Copy link

coveralls commented Nov 25, 2024

Pull Request Test Coverage Report for Build 1326189458

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.714%

Totals Coverage Status
Change from base Build 1097957204: 0.0%
Covered Lines: 120
Relevant Lines: 140

💛 - Coveralls

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