-
-
Notifications
You must be signed in to change notification settings - Fork 36
Clarifications to 'Resolved Values' section #1081
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
base: main
Are you sure you want to change the base?
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.
I don't mind changing the accessor name, but see inline comments for a few things that need fixing.
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 think the name change could be clearer?
spec/formatting.md
Outdated
@@ -155,7 +156,7 @@ and different implementations MAY choose to perform different levels of resoluti | |||
> interface MessageValue { | |||
> formatToString(): string | |||
> formatToX(): X // where X is an implementation-defined type | |||
> getValue(): unknown | |||
> getResult(): unknown |
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 think getResult
is a bit odd, given that this is an informative illustration and it is about getting the resolved value. I'm not sure what a "result" is (one might construe it to be the result of formatting, which it emphatically isn't). I would suggest either getResolvedValue
(very literal) or perhaps resolveValue
(since presumably the resolved value is the result of some sort of resolving...)
(Note that resolvedOptions()
just below should probably be resolveOptions()
instead, if we do down this path)
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.
The idea is that getResult()
returns the return value of the function handler; the resolved value is that return value, wrapped in an object that provides methods.
getResolvedValue()
doesn't seem right to me, since the resolved value is the thing implementing the MessageValue
interface, no?
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.
Since I think of MessageValue
as "wrapping" something, I thought of another name, unwrap()
, that returns the thing being wrapped. Let me know how that looks.
spec/formatting.md
Outdated
> - Using a _variable_, the _resolved value_ of an _expression_ | ||
> could be used as an _operand_ or _option value_ if | ||
> calling the `getValue()` method of its _resolved value_ did not emit an error. | ||
> calling the `getResult()` method of its _resolved value_ did not emit an error. |
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 one is strangely worded compared to the bullets just preceeding. I would suggest instead
> - Using a _variable_, the _resolved value_ of an _expression_ | |
> could be used as an _operand_ or _option value_ if | |
> calling the `getValue()` method of its _resolved value_ did not emit an error. | |
> calling the `getResult()` method of its _resolved value_ did not emit an error. | |
> - A _variable in a _declaration_ could be used as an _operand_ or _option value_ | |
> if the result of `getResult()` does not emit an error. |
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 did a slightly different rewording; let me know what you think.
Co-authored-by: Eemeli Aro <[email protected]>
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.
Given that this continues to be a non-normative example, I'm fine with unwrap()
if the general sense is that it's more informative than getValue()
.
I was re-reading the
MessageValue
example and noticed that it's easy to skip over what the last three methods are for, so I added a reference to the earlier text.I also thought that the
getValue()
method could have another name, since it's confusing to think about the "value" that it returns, vs. the enclosingMessageValue
. The name I thought of wasgetResult()
, but I'm open to other suggestions. I also thought it needed a little more explanation.