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

Suggested updates #8

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

rjzak
Copy link

@rjzak rjzak commented Jun 21, 2023

  • Remove fasthash, as it doesn't work on all platforms
  • Remove failure as it's obsolete and deprecated.
  • Add some Github actions
  • Update some dependencies
  • Modernize some aspects of the project, like impl Display instead of to_string().

Cargo.toml Outdated
fasthash= "0.4.0"
clap = "3.2"
base64 = "0.13.1"
murmurhash3 = { git = "https://github.com/malwaredb/murmurhash3-rs", branch = "update" }
Copy link

Choose a reason for hiding this comment

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

This will prevent publishing to crates.io.

Copy link
Author

Choose a reason for hiding this comment

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

I know, that's another abandoned crate.

I don't know what I was expecting with this PR. Maybe some acknowledgement from the maintainer. The abandoned crates thing bothers me.

Choose a reason for hiding this comment

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

@hdoordt Maybe you can shed some light. Maybe you can transfer maintainership or get a grant for it?

runs-on: ubuntu-latest
steps:
- name: Run Commisery
uses: tomtom-international/commisery-action@v2
Copy link

Choose a reason for hiding this comment

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

Depnding on a hash might be better here, as the tag might change at any point.

check:
runs-on: ubuntu-latest
steps:
- uses: tisonkun/[email protected]
Copy link

Choose a reason for hiding this comment

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

Same.

@lnicola
Copy link

lnicola commented Jun 22, 2023

I'm not affiliated with this repo, but TBH the two actions and Dependabot are pretty opinionated and I wouldn't have bundled them in the same PR.

},
#[fail(display = "Error: {}", msg)]
Msg { msg: String },
Io(String),
Copy link

Choose a reason for hiding this comment

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

This drops the error information, which might not be ideal.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure of the best way to store the error information, I think just one string would be fine. I don't know that various enum types are helpful. But that crate had to be removed due to it also be abandoned and having security problems.

Signed-off-by: Richard Zak <[email protected]>
@sanmai-NL
Copy link

@rjzak Thanks for your work. I'm the new maintainer. Do you have a desire to further update the PR?

@rjzak
Copy link
Author

rjzak commented Dec 22, 2023

I've decided to maintain and use my own fork, but I will address these comments.

@rjzak
Copy link
Author

rjzak commented Dec 24, 2023

I addressed the CI actions with hash, and murmurhash dependency. Let me know if you want these commits to be squashed, or if there's something else.

@sanmai-NL
Copy link

Afterwards, please don't squash since it's a mixture of unrelated changes.

@sanmai-NL
Copy link

I take it that you noticed my review.

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