search

MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.87k stars 846 forks source link

Rename BitcoinUrlBuilder to BitcoinUriBuilder #1168

Open kristapsk opened 1 year ago

kristapsk commented 1 year ago

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

Original discussion - https://github.com/MetacoSA/NBitcoin/issues/138#issuecomment-1503402357.

knocte commented 1 year ago

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

kristapsk commented 1 year ago

Rebased.

Tests pass, but, please, review again.

NicolasDorier commented 1 year ago

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

knocte commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@NicolasDorier Ok, will do that!