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

Edit of MarkM's original doc incorporating ChrisH comments #1969

Merged
merged 7 commits into from
Nov 6, 2020
Merged

Conversation

tyg
Copy link
Contributor

@tyg tyg commented Nov 3, 2020

Chris, the "alleged" you had questions about is on line 32. The comments on the Google Doc were: Chris: "Why is this 'alleged'? Was that wording used in the source doc?" and my response was Tom: "The original wording was "reporting an alleged reason for its failure"". Feel free to change what's there if you don't think it's correct.

Other general question I have is whether this is about makeNotifer/SubscriptionKits or about Notifiers and Subscriptions. If the latter, should do a quick pass to change a lot of the Kit wording to just the type.

@tyg tyg self-assigned this Nov 3, 2020
@erights
Copy link
Member

erights commented Nov 3, 2020

The original wording was "reporting an alleged reason for its failure"

Yes. The term "reason" is conventional which is why I encouraged us to adopt it and why I'm using it. But the unqualified phrase "reason for failure" suggests a stronger claim than we intend. The reason variable might be bound to a new Error('A foo is not a bar'). For reason as the name of a variable, this would then in fact be the reason for failure. But it might not actually be why the failure happened. A foo is not a bar' is the claim by some piece of code that this is why the failure happened. But that explanation might be wrong.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments so far.

I still have yet to review starting at "# SubscriptionKit"

packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
@Chris-Hibbert
Copy link
Contributor

But it might not actually be why the failure happened. A foo is not a bar' is the claim by some piece of code that this is why the failure happened. But that explanation might be wrong.

Can we say "the reason that was given for the failure"? Or "the reason the publisher gave"? It may be that we don't know whether they were right, but we do know that it was the reason we were given.

@erights
Copy link
Member

erights commented Nov 4, 2020

Can we say "the reason that was given for the failure"? Or "the reason the publisher gave"? It may be that we don't know whether they were right, but we do know that it was the reason we were given.

I like that direction. How about "reported reason"?

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review pass done.

packages/notifier/README.md Outdated Show resolved Hide resolved
@tyg
Copy link
Contributor Author

tyg commented Nov 5, 2020

Finished cleaning up PR comments.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads much better than the original. Thanks!

LGTM

packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
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