-
-
Notifications
You must be signed in to change notification settings - Fork 35
Define function composition for :string values #798
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
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
580b659
Define function composition for :string values
eemeli dd11a2c
Update spec/registry.md as suggested by @stasm in #814
eemeli 385d14b
Drop the "only"
eemeli 83289b1
Merge branch 'main' into string-composition
eemeli ed81ecd
Update text following code review comments
eemeli 3f70608
Merge branch 'main' into string-composition
aphillips File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about implementation defined namespaced options?
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.
They'd be lost, unless you're using a different
:stringthan the one we define here. If you need options, then you could/should be using:addison:string.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.
That doesn't make sense to me?
Suppose I'm @mihnita and suppose I'm writing the ICU4J implementation. Now suppose I want to support skeletons for dates. I want that to look like:
{$d :datetime icu:skeleton=yMMMdjm}, not{$d icu:datetime icu:skeleton=yMMMdjm}. Make sense so far?I don't want to namespace the core function, because I want other options to work unchanged. Obviously, my namespaced option won't work in implementations where it isn't recognized. But dropping it on the floor doesn't make sense.
Uh oh!
There was an error while loading. Please reload this page.
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.
That's a fair point; I filed #829 as a consequence of thinking through some of the implications here.
Technically, we do pass through all
:datetimeoptions so your example would be fine. For:string, I'd rather hoped that we could avoid having to deal with any options in composition.That means that something like your
addison:normalizecould still work, as long as it changed the resolved string value of the expression, rather than getting passed along separately to the next consumer of the value.Is that enough?
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
:string(or whatever:xwe define) only controls the options it owns. It's okay to say that:stringpasses none of its built-in options.I think we should say that implementation-defined or user-installed installed options MAY be passed in the resolved value or swallowed by the implementation, while unrecognized (but otherwise valid) options MUST be passed in the resolved value.
This would permit an implementation to "eat" an option so that it has no downstream effect. You'd want this on destructive options:
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.
What your example shows is possible with the currently proposed text, including an alternate version where you'd replace the second declaration with
The specific question that I think we should ask here is, "Is there any reasonable custom option for
:stringthat would not modify the input string and get swallowed up?"If we can think of at least one, then we should change the text here.
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 can't think of one for
:stringthat would behave in that way.I think we should adopt a formula for dealing with options. I would reword this as:
We might want a note to go with this to signal our intention:
Note
Custom or implementation-defined options that modify
the operand result in a modification of the resolved string value
of the operand and aren't included in the resolved value.