-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
BIP78: spelling and grammar updates #1623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clears up the language quite a bit.
I left one comment about the mixed sequence number validation step.
bip-0078.mediawiki
Outdated
@@ -189,7 +189,7 @@ The well-known error codes are: | |||
|
|||
The receiver is allowed to return implementation specific errors which may assist the sender to diagnose any issue. | |||
|
|||
However, it is important that error codes that are not well-known and that the message do not appear on the sender's software user interface. | |||
However, it is important that error codes are not well-known and that the messages do not appear on the sender's software user interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this is the intended meaning (see the lines immediately following this one). It looks to me like the idea is that error codes that are not well known should not appear in the UI and be printed in the debug log only. Also, this change would require author sign-off to ensure it doesn't alter the intended meaning of the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every spec I've seen has separate well-known error codes with templates to display to users, and a generic unknown error code for which messages they contain are only printed to debug logs. I missed that this changed themeaning in my first review. It should be reworded not to change the meaning in my view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: However, it is important that error codes and messages that are not well-known, do not appear on the sender's software user interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @remcoros! I like that suggestion. I think it brings more clarity while still preserving the meaning. Admittedly, this sentence has been a bit of a head scratcher for me. If I adopt remcoros's suggestion, I don't think it would it need BIP author sign off, right?
For now I rebased and just left this sentence alone. If there is consensus on how to reword it without changing the meaning, I am happy to open up a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satsie Happy to review your future follow-up to reword this confusing passage in a way that makes sense without changing the intended meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good modulo my comment. However, any changes that could alter the meaning of the text would need to be signed off by the BIP author. Perhaps split those out to a separate commit or PR.
Thanks for the review @jonatack! I will create a separate PR with the changes that are definitely just spelling or grammar related and rebase this (since it has comment history) to only include the ones that would need sign off from the BIP author. |
@satsie following up on the thread #1623 (comment), perhaps keep all the changes in this PR if while updating your change there it becomes clear that they are editorial-only and don't alter the meaning of the text. I'm not sure the BIP author is active in providing review feedback here at the moment. |
22610cd
to
b3445ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo this line, WDYT?
Co-authored-by: Dan Gould <d@ngould.dev> Co-authored-by: Jon Atack <jon@atack.com>
Oh that is a good catch! I accepted the suggestion and also took the advice to replace "url" with "URL". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, thanks for updating.
This PR proposes a number of spelling and grammar updates to BIP78. Nothing here should change the actual content of the BIP, though I'm new to Payjoin and would appreciate a sanity check :)
cc the BIP champion, @NicolasDorier