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

base58: Re-name crate to base58ck #2503

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

tcharding
Copy link
Member

The current name base58check is taken, as is base58. Use base58ck instead.

Add a brief section to the readme about the crate naming.

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate doc C-base58 labels Feb 25, 2024
@coveralls
Copy link

coveralls commented Feb 25, 2024

Pull Request Test Coverage Report for Build 8334199202

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 83.821%

Totals Coverage Status
Change from base Build 8318043190: 0.0%
Covered Lines: 19750
Relevant Lines: 23562

💛 - Coveralls

base58/README.md Outdated Show resolved Hide resolved
base58/README.md Outdated Show resolved Hide resolved
@tcharding tcharding force-pushed the 02-26-rename-base58 branch 2 times, most recently from 0476d7e to 203e209 Compare February 26, 2024 19:49
base58/README.md Outdated Show resolved Hide resolved
base58/README.md Outdated Show resolved Hide resolved
base58/README.md Outdated Show resolved Hide resolved
@tcharding tcharding force-pushed the 02-26-rename-base58 branch 2 times, most recently from d7e5522 to 127dcc6 Compare February 26, 2024 19:50
@tcharding
Copy link
Member Author

tcharding commented Feb 26, 2024

I removed all mention of "appears unmaintained" just to be less inflammatory, we don't know they are not maintained and we get in enough fights with the rest of the Rust community as it is.

@tcharding
Copy link
Member Author

You were reviewing while was force pushing to fix my mistakes @Kixunil, sorry about that.

apoelstra
apoelstra previously approved these changes Feb 27, 2024
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 5dc770d LGTM. I think it is fine to say "unmaintained" but understand the desire not to start fights

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 27, 2024

"appears unmaintained" just to be less inflammatory, we don't know they are not maintained and we get in enough fights with the rest of the Rust community as it is.

If anyone is offended by simply stating that their crate which is not 1.0 didn't update for years and didn't respond to messages appears unmaintained then better know that they're that kind of person. You can't please everyone and I would definitely not go out of my way to please those kinds of people.

Also provided reasonable people, there is not problem with updating it later.

@tcharding
Copy link
Member Author

Fair points, will add it back it.

apoelstra
apoelstra previously approved these changes Feb 27, 2024
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 7d57564 though "where as" should be one word

@tcharding
Copy link
Member Author

Will fix to use correct spelling of whereas.

apoelstra
apoelstra previously approved these changes Feb 27, 2024
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK dffd566

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 28, 2024

Looks like the maintainer of base58 is willing to transfer ownership, so let's just wait if he does.

@tcharding
Copy link
Member Author

I kind of think we should just use the new name because of how the current base58check crate depends on base58 and sha2. Our base58 crate is quite different.

@tcharding
Copy link
Member Author

Its not like we are trying to sell our crate to the world, its mainly just so rust-bitcoin can use it. People who want it will find it.

@apoelstra
Copy link
Member

I have the same feeling -- we should leave base58 and base58check as the existing pair.

But let's wait a bit to see what the base58 maintainer thinks.

@apoelstra
Copy link
Member

In 76aa10b if you are going to add the word whereas it needs to be preceded by a comma, not a semicolon. Otherwise ACK.

@tcharding
Copy link
Member Author

If only I spoke English as a first language, oh wait ... :(

The current name `base58check` is taken, as is `base58`. Use `base58ck`
instead.

Add a brief section to the readme about the crate naming.
@tcharding tcharding force-pushed the 02-26-rename-base58 branch 2 times, most recently from bfe167a to 6b09857 Compare March 18, 2024 22:01
@tcharding tcharding added the P-high High priority label Mar 18, 2024
@tcharding tcharding requested a review from Kixunil March 19, 2024 01:59
@tcharding tcharding dismissed Kixunil’s stale review March 19, 2024 02:00

Implemented as suggested.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 6b09857

@tcharding
Copy link
Member Author

All the github stuff about requesting review from @Kixunil was just from me getting rid of requested changes because they are all implemented, did not mean to imply we are waiting on him. @sanket1729 have you a minute to review this one please, its trivial.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 6b09857

@apoelstra
Copy link
Member

Gonna merge this. Note that we will not cut a release off this PR. At the very least we also need #2481.

@apoelstra apoelstra merged commit 6ff8505 into rust-bitcoin:master Mar 20, 2024
187 checks passed
@tcharding
Copy link
Member Author

Yep, release tracking PR is up in draft state already #2595

@tcharding tcharding deleted the 02-26-rename-base58 branch March 20, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-base58 C-bitcoin PRs modifying the bitcoin crate doc P-high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants