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

Improved docs, fixed clippy warnings, fixed cargo fmt #42

Merged
merged 5 commits into from Jan 12, 2020

Conversation

@duesee
Copy link
Contributor

@duesee duesee commented Jan 1, 2020

When reading the code I made some minor improvements I wanted to share. Notably, I tried to make the usecase for Bech32 more clear and fix all clippy warnings and "incorrect" formatting.

Have you thought about introducing a CI (e.g., via GitHub actions) which checks against cargo audit, cargo clippy and cargo fmt?

@clarkmoody
Copy link
Member

@clarkmoody clarkmoody commented Jan 3, 2020

Interesting. I'll circle back after #41 is merged. We'll probably need a rebase then as well.

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Jan 3, 2020

The changes look good, some transformations from index to iterator loops might even make the code faster.

The only problem I don't have a good solution for right now is how to handle the rustfmt::skip attribute in older rust versions (before macros were scoped). Currently it makes our tests fail for rust 1.22. Maybe we can make the attribute conditional and only run rustfmt with more recent rust versions. But I fear the error occurs when parsing the AST, so even disabling the code with cfg won't help. What we could do in that case is removing the attribute using sed when running the 1.22 test since it isn't needed there anyway, but that's an ugly hack.

If we accept rustfmt changes we should definitely add a rustfmt check to our already existing CI pipeline otherwise non-conforming code will be checked in again. Clippy is a bit of a problem since not all fixes are compatible with older rust versions. cargo audit seems most useful as a cron job and not for every PR, we could activate weekly builds in travis and only run it on master.

@clarkmoody
Copy link
Member

@clarkmoody clarkmoody commented Jan 3, 2020

Needs rebase.

@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 4, 2020

Is there a reason to support Rust 1.22? Maybe it could be edition = '2018' anyway? :-)

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Jan 6, 2020

The rationale for older rust versions is given in rust-bitcoin/rust-bitcoin#338, although I can't remember why it's exactly 1.22. 1.24 seems to be the version we really want to support, but I fear that won't help with our problem.

@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 6, 2020

I don't see any easy way of supporting rustfmt's skip. #[rustfmt::skip] does not work in 1.22.0, Clippy lints on #[cfg_attr(rustfmt, rustfmt_skip)] and I assume #[warn(clippy::deprecated_cfg_attr)] will not work in 1.22 either nor should it be used. I just bypassed the formatting so that not to generate too much hassle with this PR. I don't know if CHARSET_REV needs any extra formatting, but if so, I guess it can be done in a similar fashion.

@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 6, 2020

Regarding cargo audit: having it run on every PR prevents pulling in already vulnerable dependencies -- who knows? :-)

Copy link
Member

@sgeisler sgeisler left a comment

The changes already made look good to me. But to preserve the formatting over future PRs we really need to integrate rustfmt into our build pipeline (only for stable I guess). Do you know your way around travis? If not I could do it so we can merge the PR.

@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 9, 2020

I would be glad if you could do it :-)

@sgeisler sgeisler force-pushed the duesee:master branch 2 times, most recently from 4f77f79 to 0a7bcb1 Jan 11, 2020
Copy link
Member

@sgeisler sgeisler left a comment

@duesee I'd be glad if you could take a look at the CI tests I added. I think they should check all the changes you did in this PR.

From my side it looks merge ready now. @clarkmoody what do you think?

- rustup component add rustfmt
- rustup component add clippy
script:
- rustfmt --check src/lib.rs

This comment has been minimized.

@sgeisler

sgeisler Jan 11, 2020
Member

Is there a better way to do this? Apparently cargo fmt doesn't support the --check flag, so I have to use rustfmt directly and specify the files to check.

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Jan 11, 2020

Oh, I just noticed that there are merge conflicts now. I'll try to rebase.

duesee added 2 commits Jan 1, 2020
* why is Bech32 used?
* mention "1" as separator
* refer explicitly to original description *for more details*
@sgeisler sgeisler force-pushed the duesee:master branch from 0a7bcb1 to ea64441 Jan 11, 2020
@sgeisler sgeisler force-pushed the duesee:master branch from ea64441 to 71e0c2a Jan 11, 2020
@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 11, 2020

Thank you @sgeisler! The CI should catch everything now. Having cargo audit run on every push might still be a good (and easy to implement) idea. But it could be added at a later time and issue.

@sgeisler
Copy link
Member

@sgeisler sgeisler commented Jan 12, 2020

Copy link
Member

@clarkmoody clarkmoody left a comment

This works for me.

@sgeisler We can update the Travis script with something more clever later, if need be.

@clarkmoody clarkmoody merged commit a565895 into rust-bitcoin:master Jan 12, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sgeisler
Copy link
Member

@sgeisler sgeisler commented Jan 14, 2020

I though about cargo audit again and it doesn't make much sense for us anyway since we don't have any dependencies.

@duesee
Copy link
Contributor Author

@duesee duesee commented Jan 14, 2020

You don't have dependencies now ;-)

@clarkmoody
Copy link
Member

@clarkmoody clarkmoody commented Jan 15, 2020

I think it should be a long-term goal that this library has no dependencies...

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

Successfully merging this pull request may close these issues.

None yet

3 participants