Skip to content

transaction: Rename is_coin_base to is_coinbase#1796

Merged
apoelstra merged 3 commits intorust-bitcoin:masterfrom
stevenroose:rename-is-coinbase
Apr 24, 2023
Merged

transaction: Rename is_coin_base to is_coinbase#1796
apoelstra merged 3 commits intorust-bitcoin:masterfrom
stevenroose:rename-is-coinbase

Conversation

@stevenroose
Copy link
Copy Markdown
Collaborator

@stevenroose stevenroose commented Apr 13, 2023

Alternative to #1795.

Keep the old method as deprecated and add doc alias. Also change internal usage of the method.

@stevenroose stevenroose mentioned this pull request Apr 13, 2023
Closed
Kixunil
Kixunil previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK b2e90eaabfa7c7683882484ae5dfc137f787190f

I prefer this to the alternative.

@Kixunil Kixunil added API break This PR requires a version bump for the next release 1.0 Issues and PRs required or helping to stabilize the API labels Apr 13, 2023
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Apr 13, 2023 •
edited
Loading

Lolll looks like we need to stick allow(clippy::deprecated_semver) on the crate to use the NEXT_VERSION trick. Or use something like 0.0.0-NEXT_VERSION.

tcharding
tcharding previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 5ad3da256ba221f945be275ac2e6dba0398f6950

bitcoin/src/blockdata/transaction.rs Outdated
Copy link
Copy Markdown
Member

@tcharding tcharding Apr 13, 2023

Choose a reason for hiding this comment

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

Interesting, is the idea to remove this when we remove the deprecated function? I've never used this attribute myself.

Copy link
Copy Markdown
Collaborator Author

@stevenroose stevenroose Apr 19, 2023

Choose a reason for hiding this comment

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

I'd keep it for as long as we can tolerate it. Anyone might remember the old name and search for it. It doesn't hurt I guess, it only makes it show up in search when you search for that name.

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I wouldn't mind keeping it forever. Not just because it had this name but also that it's a name someone might think of.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Apr 13, 2023

Looks like "NEXT_RELEASE" will need to go in in the "note = NEXT RELEASE" field. We should grep for "NEXT.RELEASE" too so we don't miss "NEXT-RELEASE" or "NEXT RELEASE", and leave that in released code which would be pretty embarrassing.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Apr 14, 2023

We could also allow the lint in non-release PRs and enable it in release PRs.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Apr 14, 2023

We could also allow the lint in non-release PRs and enable it in release PRs.

I like this. I think we do it by cargo clippy -- -D clippy::deprecated_semver ... though I tried to test this, and we don't have any globally-allowed lints I can test this on, and it seems like it doesn't override locally-allowed ones.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

stevenroose commented Apr 19, 2023

Grrrr at clippy enforcement, it's so unproductive! So I will try to silence clippy for non-release CI then? Is that the consensus? Let me see if I can figure out how to do that.

@stevenroose
transaction: Rename is_coin_base to is_coinbase
dad3abd 
Keep the old method as deprecated and add doc alias.
Also change internal usage of the method.
@stevenroose
Copy link
Copy Markdown
Collaborator Author

stevenroose commented Apr 19, 2023

Hmm, I'm not sure how the CI is built, but running clippy in the release job would require having nightly cargo in that job and I don't think we want that?

I went for what Tobin suggested instead, renaming to NEXT-RELEASE and matching for that. That seems to be accepted by overlord clippy nightly as a valid semver version. Tbh I think I've seen underscores in semvers, I don't see the issue. I tested the grep and for me it worked.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Apr 19, 2023

Why would nightly be a problem? Sure there's a risk that release gets stalled a bit due to a nightly bug but worst case we could turn it off.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

stevenroose commented Apr 19, 2023

Why would nightly be a problem? Sure there's a risk that release gets stalled a bit due to a nightly bug but worst case we could turn it off.

Nightly is generally a problem, right? What if a regression in nightly actually makes the release go through while it shouldn't? In fact I'd say it makes most sense to test the release in MSRV. Idk exactly what the release command checks for and how it differs from cargo build, but I can imagine things like new manifest fields or something be a thing. We don't have Cargo.lock, so that should be ok.

Anyway, I think like it's done in my latest commit should make sense.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Apr 19, 2023

I believe we should test on all versions.

tcharding
tcharding previously approved these changes Apr 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 38d11ce

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Apr 19, 2023 •
edited
Loading

CI fail is a formatting issue in patch 1 bro. Your favourite :)

@stevenroose
Copy link
Copy Markdown
Collaborator Author

stevenroose commented Apr 24, 2023

pushed a fix

tcharding
tcharding approved these changes Apr 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK a54e1ce

apoelstra
apoelstra approved these changes Apr 24, 2023
View reviewed changes
Copy link
Copy Markdown
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 a54e1ce

@apoelstra apoelstra merged commit 84a075d into rust-bitcoin:master Apr 24, 2023
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Apr 26, 2023

Lol, just noticed the author of the last commit.

@realeinherjar realeinherjar mentioned this pull request Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees

No one assigned

Labels

1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stevenroose @apoelstra @tcharding @Kixunil