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

Various lints #30

Closed
wants to merge 4 commits into from
Closed

Various lints #30

wants to merge 4 commits into from

Conversation

a-dma
Copy link

@a-dma a-dma commented Oct 3, 2017

Hi,

I've added a few lints to clean up the code. Tests still pass.
The most notable change is that I have broken the public API, recover_secret now takes a slice rather than a Vec.

A couple more things that I didn't fix but that should be addressed:

  • The protobuf method make_repeated_bytes_accessor is deprecated. Any reason why that is still used?
  • In wrapped_secrets.rs:31 there is a weird for loop over an Option<String>. Is that on purpose? What are you trying to do? I think you're better off with if let Some(mt)
  • I didn't want to ruin the whole history and send a million lines PR, but you should really consider to format the code using rustfmt

One last thing, are you planning a release of the signed shares stuff?

@romac
Copy link
Member

romac commented Oct 4, 2017

Cue, copy-pasta of my comment on #29:

First of all, sorry for the late reply, and thank you very much for the PR!

Sadly, I won't be able to merge it, as we are currently working on a big refactoring (+ more cool features), which encompasses the ones you submitted. We will release those changes publicly very soon (at least as a PR open for comments), and thus hope that you will not be too disappointed by the negative reply (which has nothing to do with your PR as such). We always welcome contributions, new issues, new ideas, and criticisms, and are looking forward to hearing your input on the upcoming pre-release!

Thanks again!

PS: I realise that the message above sounds very "corporate", sorry about that, it wasn't my intention when writing it up. I'm just not very good at interacting with people online…

Now onto the issues you raised:

Hi,

I've added a few lints to clean up the code. Tests still pass.

Awesome! This has already been taken care as part of the changes mentioned above, but it's always good to see people actually building the code and looking at the build output :)

The most notable change is that I have broken the public API, recover_secret now takes a slice rather than a Vec.

As it will as well (amongst other things) in the next release :)

The protobuf method make_repeated_bytes_accessor is deprecated. Any reason why that is still used?

Only that that code was generated by a previous version of rust-protobuf, and that we hadn't gotten around to update it.

In wrapped_secrets.rs:311 there is a weird for loop over an Option. Is that on purpose? What are you trying to do? I think you're better off with if let Some(mt)`

Absolutely, it's been fixed as well (thanks Clippy :))

I didn't want to ruin the whole history and send a million lines PR, but you should really consider to format the code using rustfmt.

Of course! It's been done as well, and will soon be enforced on Travis.

One last thing, are you planning a release of the signed shares stuff?
Yup, it's coming as well. Please bare with us while we are finishing up the next (pre-)release, and thanks again for your input!

@romac romac closed this Oct 4, 2017
@a-dma
Copy link
Author

a-dma commented Oct 5, 2017

No worries. I understand what you're saying and it wasn't much work to begin with. So no harm in tossing it.

I'll take the opportunity to ask a couple more questions, I hope you don't mind:

  • Have you considered using serde rather than protobuf for the serialization?
  • Do you have a rough ETA on the version?

Thanks and I'll be looking forward to a v1.0.0 ;)

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.

None yet

2 participants