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

Misc changes #121

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Misc changes #121

wants to merge 3 commits into from

Conversation

rrtoledo
Copy link
Collaborator

@rrtoledo rrtoledo commented Jan 14, 2025

Content

This PR includes:

  • Moving /docs to .gitignore
  • Moving Hashes in utils
  • Fixing clippy

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)

Comments

Misc changes from from former #66 and #116

Issue(s)

Relates to #59

@rrtoledo rrtoledo self-assigned this Jan 14, 2025
Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

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

LGTM 👍
One thing: I have changed the DATA_LENGTH = 48.
However, @tolikzinovyev said: If the data length is changed, it should be done in example PRs, see.
If we go with the way that Tolik suggested, I will re-set the data length to 32. If not, you may want to update this branch :)

@curiecrypt curiecrypt mentioned this pull request Jan 15, 2025
9 tasks
@@ -4,3 +4,4 @@ dist-newstyle
.DS_Store
target
Cargo.lock
docs
Copy link
Member

Choose a reason for hiding this comment

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

Why this? docs is committed in the repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that docs should be built locally and not committed

let params = Params {
soundness_param: 10.0,
completeness_param: 10.0,
set_size: 80 * set_size / 100,
lower_bound: 20 * set_size / 100,
set_size: set_size.saturating_mul(80).div_ceil(100),
Copy link
Member

Choose a reason for hiding this comment

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

Does this trigger for you when you run clippy? I doesn't on my laptop or in CI, even though it should...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It did trigger on the CI in the old PR

@rrtoledo
Copy link
Collaborator Author

rrtoledo commented Jan 16, 2025

LGTM 👍 One thing: I have changed the DATA_LENGTH = 48. However, @tolikzinovyev said: If the data length is changed, it should be done in example PRs, see. If we go with the way that Tolik suggested, I will re-set the data length to 32. If not, you may want to update this branch :)

Rebased

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