Skip to content

Formatting: Don't remove unresolved options from the map passed to the function #545

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

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

stasm
Copy link
Collaborator

@stasm stasm commented Nov 29, 2023

A follow-up to #524 (comment):

@stasm:

I see that this step was added yesterday in a suggestion from @gibson042. What is the purpose of it? Why wouldn't we want functions to see unresolved options?

@eemeli:

This is a small fix unrelated to namespacing that's bundled in here to ensure that if

{:foo opt=$none opt=bar}

is formatted with $none not resolving, not only will that cause an Unresolved Variable error to be emitted, but that a Duplicate Option Name error will also be emitted -- the latter was previously skipped.

The behaviour around options with errors is unchanged from the previous; it's just expressed a bit differently. If you think that ought to be changed, then that should go in a different PR as this is scope-creepy enough already.

Perhaps I'm missing something, but my reading of the current step 3 is that both errors would be emitted. I don't really see how step 4, which I remove in this PR, helps.

Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but I had one comment.

@@ -217,8 +217,8 @@ the following steps are taken:
bind the _identifier_ of the _option_ to the resolved value in the mapping.
- Otherwise, bind the _identifier_ of the _option_ to an unresolved value in the mapping.
(Note that an Unresolved Variable error will have been emitted.)
4. Remove from the resolved mapping of _options_ any binding for which the value is an unresolved value.
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable, but perhaps we should explicitly allow implementations to remove the value later (to prevent the local equivalent of NPE):

Suggested change
4. Remove from the resolved mapping of _options_ any binding for which the value is an unresolved value.
Implementations MAY later remove this value before calling the _function_.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this suggestion, thanks @aphillips.

I think delegating the decision about whether to remove or not to implementations is the right choice. In some implementations it's going to be natural to use a sentinel value to represent unresolved option values; other implementations may not have such possibility and they can instead choose to completely remove unresolved options from the map before it's passed to the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't able to apply this suggestion via GitHub's UI. I pushed 006e60d instead.

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I'm not convinced that it's valuable to allow an implementation to see that an option was provided but has no associated value, especially because the "value" in such a case must be distinct from anything that can be specified. How do you anticipate the information being used?

@aphillips aphillips added specification Issue affects the specification formatting Issue pertains to the formatting section of the spec labels Dec 1, 2023
@stasm
Copy link
Collaborator Author

stasm commented Dec 13, 2023

I'm not convinced that it's valuable to allow an implementation to see that an option was provided but has no associated value, especially because the "value" in such a case must be distinct from anything that can be specified. How do you anticipate the information being used?

In implementations that don't remove unresolved option values, custom functions may provide better error reporting experience by being able to differentiate between a missing option and an unresolved option.

Consider a function which expected an option called required:

{:func required=$error}

If required is removed by the implementation before func is invoked, the error reported by func may state that "required was not passed". This may degrade the developer experience: after all, the option was passed, and the source of the error is something else.

@stasm stasm requested review from aphillips and gibson042 December 13, 2023 20:42
Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Works for me; thanks!

@aphillips aphillips merged commit 4abc384 into main Dec 15, 2023
@aphillips aphillips deleted the dont-remove-unresolved-options branch December 15, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting Issue pertains to the formatting section of the spec specification Issue affects the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants