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

Rename BitcoinUrlBuilder to BitcoinUriBuilder #1168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kristapsk
Copy link

BIP21 contains URIs, not URLs, this is correct terminology (every URL is URI, but not every URI is URL).

Original discussion - #138 (comment).

@knocte
Copy link
Contributor

knocte commented May 18, 2023

utACK, however, @kristapsk unfortunately this PR needs to be rebased

@kristapsk kristapsk force-pushed the bip21-rename-url-uri branch from ca19a03 to 6bcf13c Compare May 18, 2023 09:02
@kristapsk
Copy link
Author

Rebased.

Tests pass, but, please, review again.

@NicolasDorier
Copy link
Collaborator

I am on the fence for this one. This might requires a major release as it's a breaking change.

@knocte
Copy link
Contributor

knocte commented May 19, 2023

as it's a breaking change.

Are you sure? Previous consumers of the API will still work because he left a type called BitcoinUrlBuilder there. Only things that NBitcoin consumers will notice is some Obsolete warnings.

@augustoproiete
Copy link
Contributor

as it's a breaking change.

Are you sure? Previous consumers of the API will still work because he left a type called BitcoinUrlBuilder there. Only things that NBitcoin consumers will notice is some Obsolete warnings.

It's a binary breaking change for sure. The old class now inherits from the new class, so it's no longer the same type across different versions of NBitcoin, so if this were to be merged, it should be released with a major version bump.

An easy way to solve this would be remove the inheritance and use encapsulation instead... Just forwarding all the methods in the old class, to the new class... That would allow for a smooth minor version bump.

@NicolasDorier
Copy link
Collaborator

NicolasDorier commented May 20, 2023

@kristapsk can you take simpler approach, not breaking anything: Just leave the BitcoinUrlBuilder as it was before, just add the [Obsolete] attribute like you did. No inheritance.

Then just duplicate the code for BitcoinUriBuilder. I will eventually remove BitcoinUrlBuilder in a major release.

Would fix breaking change @augustoproiete pointed out.

@kristapsk
Copy link
Author

@NicolasDorier Ok, will do that!

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.

5 participants