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

doc: removed dead links #3513

Draft
wants to merge 1 commit into
base: 1.15.0-dev
Choose a base branch
from

Conversation

daank-c
Copy link
Contributor

@daank-c daank-c commented Apr 7, 2024

This removes 3 dead links from the shared-libraries.md document and adds some missing punctuation.

removes dead links from shared-libraries document and fixes some punctuation.
@patricklodder
Copy link
Member

Although this is cool, I'm not sure if this documentation is correct. Looks like it was never ported, just someone doing sed s/bitcoin/dogecoin/g on it. I'll go figure out what is actually real in this and what isn't if you don't mind.

Re: punctuation. Note that none of the list items are sentences, so normally those don't get punctuation?

@patricklodder patricklodder requested a review from a team April 7, 2024 18:59
@patricklodder patricklodder added this to the 1.15.0 milestone Apr 7, 2024
@patricklodder patricklodder added this to 🚀 needs review in Review & merge board Apr 7, 2024
@daank-c
Copy link
Contributor Author

daank-c commented Apr 7, 2024

I'll go figure out what is actually real in this and what isn't if you don't mind.

I don't mind at all.

Note that none of the list items are sentences, so normally those don't get punctuation.

The 'Parameters' list above the 'Script Flags' and 'Errors' lists uses punctuation, so I made the punctuation consistent across all lists. I can restore the punctuation back to how it was, if that's preferred.

@patricklodder
Copy link
Member

I can restore the punctuation back to how it was, if that's preferred.

Let's see what in those lists is actually real documentation for this repo first.

@patricklodder
Copy link
Member

patricklodder commented Apr 7, 2024

So, this document does not reflect implementation, at all. The replacement of bitcoin with dogecoin has not been reflected at all in the actual header file. I do think that the symbol names reflected in the documentation are desirable over the symbols actually exported, especially because these would conflict with someone importing both libbitcoinconsensus and libdogecoinconsensus 😅

So as of right now, I would not trust anything this document says. I see 2 options:

  1. Fix the library to actually use correct symbols (and make sure it actually works)
  2. Deprecate the whole library and the docs. I do not know anyone that currently uses it - it's not really useful anyway.

@daank-c
Copy link
Contributor Author

daank-c commented Apr 7, 2024

I would be fine with deprecating the whole library and documentation if it's not being used.

Looks like bitcoin is doing the same bitcoin/bitcoin#29189.

@patricklodder
Copy link
Member

Looks like bitcoin is doing the same

Bitcoin Core is replacing this library with another, more complete library, libbitcoinkernel.

@daank-c
Copy link
Contributor Author

daank-c commented Apr 8, 2024

Do we have any plans to do the libbitcoinkernel thing to Dogecoin in the future? (seems like a lot of work)

@patricklodder
Copy link
Member

Do we have any plans to do the libbitcoinkernel thing to Dogecoin in the future? (seems like a lot of work)

Personally, I think that having a well maintained shared library that exposes this codebase could help when testing alternative implementations and therefore would expect such a request from serious alternatives and auditors. But, having a non-maintained library like the one we have now (where for almost 5 years its docs could not have been more misleading, because every symbol is misnamed) is however a waste of everyone's time and effort.

So unless someone is going to use it, I'd not prioritize that.

@patricklodder
Copy link
Member

I'm going to change this to draft and take it off the review board for now, because whatever we do, this change on its own doesn't make sense - i.e. will need to do work either way. However, since we already started the discussion here, we might as well have the discussion here, and give people some time to think about their wants/needs wrt this, and react.

@patricklodder patricklodder marked this pull request as draft April 8, 2024 19:04
@patricklodder patricklodder removed the request for review from a team April 8, 2024 19:04
@patricklodder patricklodder removed this from the 1.15.0 milestone Apr 8, 2024
@patricklodder patricklodder removed this from 🚀 needs review in Review & merge board Apr 8, 2024
@daank-c
Copy link
Contributor Author

daank-c commented Apr 9, 2024

Sounds good, and I agree that it needs work either way.

Just one note: if a solution isn't found before the next release, it would make me more comfortable if there was at least a label at the top of the document saying "this library is not actively maintained" or something to make it obvious that it's not useful at the moment. I just wouldn't want to see anyone waste their time or become confused by reading it.

@patricklodder
Copy link
Member

Documented a reminder in #3515

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

Successfully merging this pull request may close these issues.

None yet

2 participants