Skip to content

Added more words for Denomination parser#593

Closed
keblek wants to merge 1 commit intorust-bitcoin:masterfrom
keblek:patch-1
Closed

Added more words for Denomination parser#593
keblek wants to merge 1 commit intorust-bitcoin:masterfrom
keblek:patch-1

Conversation

@keblek
Copy link
Copy Markdown

@keblek keblek commented Apr 12, 2021

I noticed that this is more flexible to parse user input with, when I was making a small CLI with.

I noticed that this is more flexible to parse user input with, when I was making a small CLI with.
@apoelstra
Copy link
Copy Markdown
Member

concept NACK. This sort of almost-natural-language processing does not belong in this library.

@keblek
Copy link
Copy Markdown
Author

keblek commented Apr 29, 2021

Fair enough, @apoelstra if I trimmed it down to fewer words would it be more appropriate?

Maybe a good compromise would be;

"msat" | "millisatoshi" => Ok(Denomination::MilliSatoshi)

This way a user of the library can tokenise an input string, make it lowercase and have the library parse it with ease.

@sgeisler
Copy link
Copy Markdown
Contributor

Sorry, not a big fan either, I was quite surprised to see we support anything apart from BTC and sats at all 😄

Looking at the string parsing code with denomination I'm quite unsure how useful it actually is. Who uses this? The only impl I know that works with units in the API is c-lightning. But that uses msat (that can't be represented by our amount type unless it's only whole sats) and also has no space between number and denomination, so our parsing code would fail anyway afaik.

@keblek
Copy link
Copy Markdown
Author

keblek commented Apr 29, 2021

Well I was trying to use it. I guess I didn't really know how this library was supposed to be used. Seeing all the matching arms I figured that it was meant for parsing.

If that's not the case I would prefer just closing this as it is so unimportant and minor that I would prefer that it not take up more time.

@keblek keblek closed this Apr 30, 2021
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.

3 participants