Navigation Menu

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

Clarify the order of $f[opt-names] and $f[opt-values] #1631

Open
dunsany opened this issue Dec 11, 2022 · 11 comments
Open

Clarify the order of $f[opt-names] and $f[opt-values] #1631

dunsany opened this issue Dec 11, 2022 · 11 comments

Comments

@dunsany
Copy link
Contributor

dunsany commented Dec 11, 2022

Currently those values in the function pseudo-map contain lists in the order exactly as defined - in contrast to make-map, for example, which seems to order its keys alphabetically:

~> put [c 3] [a 1] [b 2] | make-map[&a=1 &b=2 &c=3]
~> put {|&c=3 &a=1 &b=2|}[opt-names][c a b]

I feel that this should be clarified in the documentation. Map is defined as "a value containing unordered key-value pairs", so the first result is to be (not) expected. But since the "mapping" of option values and names is supplied in the pseudo-map as two lists, it feels like it should match the definition order. Either way, clarification would be much appreciated.

For context, my current use-case are with-statements (as in Python). One example:

 fn with-temp-dir { |&parent='' f|
    use path
    use util

    var @args = (each { |name|
        path:temp-dir &dir=$parent $name'-*'
    } $f[arg-names])
    var @opt_values = (each { |name|
        path:temp-dir &dir=$parent $name'-*'
    } $f[opt-defaults])

    defer {
        { all $args; all $opt_values } | each { |dir|
            rm -rf $dir
        }
    }

    var opts = (util:zip $f[opt-names] $opt_values | make-map)
    call $f $args $opts
}
~> with-temp-dir {|cyan magenta| put $cyan $magenta}
▶ /tmp/cyan-3550046558
▶ /tmp/magenta-93444558
~> with-temp-dir {|yellow &dark=black| put $yellow $dark}
▶ /tmp/yellow-3399318214
▶ /tmp/black-2816232334

(by the way, any critique/suggestions regarding this are much appreciated, but having possibility of defining things like this feels really nice)

I'm sorry for ruining 256 open issues for something this trivial. :-)

@krader1961
Copy link
Contributor

It's unclear what you're proposing. The documentation states that at this time iterating over the keys of a map, pseudo or otherwise, should be treated as unordered. The fact that there is an ordering for pseudo maps is an implementation detail that doesn't negate the advice to treat them as if their keys are unordered since, in general, you are unlikely to be aware which type of map you're working with. See issue #1025 for one discussion (there are probably others) regarding whether maps should have a predictable order when enumerating their keys.

Note that as I write this the consensus seems to be to change the behavior to use the insertion order, rather than sorting the keys, if a non-random ordering is desired. Similar to Python's behavior introduced in its 3.7 release and available in prior versions using its collections.OrderedDict implementation.

@hanche
Copy link
Contributor

hanche commented Dec 21, 2022

This seems like a non-issue to me. The important bit is the identical ordering of $f[opt-names] and $f[opt-defaults], which is well documented.

But personally, if we are now in bikeshedding mode, I think it would have made better sense to provide $f[options] as a (pseudo-)map mapping the names of options to the corresponding default values.

@dunsany
Copy link
Contributor Author

dunsany commented Dec 21, 2022

Maybe the example that I posted is not specific enough. I can always post a use-case that depends on having those lists in the order as the arguments were defined. I don't really think this qualifies as "bikeshedding", though. If you feel this isn't important to you, @hanche, it's fine, but I'm not sure why you troubled yourself to communicate it here.

I'm also not sure why my suggestion isn't clear to @krader1961, but I'll try to rephrase it: right now, the order of arguments in those two lists matches the order in which these arguments were defined. I'd like it to stay this way and be a documented behaviour.

The documentation states that at this time iterating over the keys of a map, pseudo or otherwise, should be treated as unordered.

This isn't iterating over keys of a map. It's iterating over two lists, which have a stable order in and of itself. I'm under the impression that access to function's options was designed in this slightly peculiar way exactly on purpose. I just want this purpose documented and make this behaviour reliable. If it were a map instead, like @hanche suggests, then we'd have a problem with keys order. But right now we don't, and the matter of map keys ordering is irrelevant to my suggestion.

@krader1961
Copy link
Contributor

@dunsany: It was unclear to me because you wrote "Map is defined as a value containing unordered key-value pairs" and a couple of similar statements. Implying you were bothered by the behavior of iterating over map keys. In other words, your original problem statement, especially your examples involving make-map, makes it hard to understand your point. Also, the documentation explicitly addresses your point regarding the order of the two lists:

$f[opt-names] is a list containing the names of the options.

$f[opt-defaults] is a list containing the default values of the options, in the same order as $f[opt-names].

You wrote "If it were a map instead, like @hanche suggests, then we'd have a problem with keys order." No, we would not. There is no explicit guarantee that the $f[opt-names] list is in the order the options were defined. Today that correspondence is true but it isn't documented to be true now or in the future and it's not clear the order needs to match the order the options were defined. Too, the order the options were defined is not the same thing as the order the arguments appear when the function is invoked; the latter being important sometimes (think something like the find command).

@dunsany
Copy link
Contributor Author

dunsany commented Dec 22, 2022

@krader1961: we apparently still have some communication problems. Let's try to rectify them.

your original problem statement, especially your examples involving make-map, makes it hard to understand your point

I only included the make-map bit because of the syntax and form analogy — option names and values are obviously some type of mapping, and I wanted to show that I'm not after order in maps in general (even though these lists are not maps).

Also, the documentation explicitly addresses your point regarding the order of the two lists

This wasn't my point. I'm aware that the $f[opt-defaults] order corresponds to $f[opt-names] order (it's obvious, after all, as it wouldn't make any sense if it didn't).

There is no explicit guarantee that the $f[opt-names] list is in the order the options were defined. Today that correspondence is true but it isn't documented to be true now or in the future

Now that is my point exactly, as stated here:

right now, the order of arguments in those two lists matches the order in which these arguments were defined. I'd like it to stay this way and be a documented behaviour.


it's not clear the order needs to match the order the options were defined

If we decide that order doesn't need to match the definition order, then it will become a less useful introspection feature and we can use just map instead to not confuse anyone (why have two lists when you have keys and values functions)? But right now we have two lists, and that's why I wrote this:

I'm under the impression that access to function's options was designed in this slightly peculiar way exactly on purpose. I just want this purpose documented and make this behavior reliable.

By the way, @xiaq, could you comment on your intentions here?


the order the options were defined is not the same thing as the order the arguments appear when the function is invoked

As this is a function introspection feature, nobody would expect this — when you analyze a function object, there is obviously no way to know how it will be used in the future. The find command example is something completely different (which doesn't fit Elvish options design at all right now).

@krader1961
Copy link
Contributor

As this is a function introspection feature, nobody would expect this — when you analyze a function object, there is obviously no way to know how it will be used in the future.

I still fail to see why the order of the options in the $f[opt-names] list needs to match to order the options were defined. If I execute these statements I don't see why the output needs to match the order the options were defined:

elvish> fn f {|&c=3 &a=1 &b=2 x| put $x }
elvish> put $f~[opt-names]
▶ [c a b]

It is obviously reasonable for the order to match that of the options in the definition but I fail to see why it matters if it doesn't match. What problems occur if the order does not match? Put another way, If @hanche's proposal was implemented, and the order of the keys in the $f[options] map was random, what practical significance would that have?

@dunsany
Copy link
Contributor Author

dunsany commented Dec 22, 2022

It can matter in some applications. My design is a little bit weird, but OK, here it comes.

Remember that context manager from first post? Let's imagine that we use that design to create a temporary OverlayFS mountpoint. In other words, we want to give a directory that needs to be overlayed and get, for example, only mountpoint and upper directories, both temporary ones, created for this scope only. So the function would be called like this:

with-temp-overlay /home/dunsany/bottom_dir { |mountpoint upper|
    put $mountpoint
    put $upper
}

But let's make it more fancy. Let's say we want the temporary directories created for us have meaningful names. We can get those directly from the "callback" function! So like this:

~> with-temp-overlay /home/dunsany/bottom_dir { |decoy changes|
       put $decoy
       put $changes
   }
▶ /tmp/decoy-123456
▶ /tmp/changes-123457

Now let's make it even more fancy. Let's say we have some names in mind for our temporary directories, but we don't want to use them as variable names inside our context-managed scope. So we'd like to do this:

~> with-temp-overlay /home/dunsany/bottom_dir { |&mountpoint=decoy &upper=changes|
       put $mountpoint
       put $upper
   }
▶ /tmp/decoy-654321
▶ /tmp/changes-654320

You can see why the order matters. Our context manager, with-temp-overlay, allows us here to pass two pieces of information for every argument, but it expects those arguments in certain order. Of course, if the context manager accepts scopes with two arguments, we can't use option for only the first one, but we could use it only for the second.

@krader1961
Copy link
Contributor

You can see why the order matters.

No, I can't. 😄 The order of options, pretty much by definition, does not matter. It seems to me you are misusing options where ordered arguments are the appropriate solution. It's possible I'm missing something obvious but so far you have failed to illustrate, at least to me, why the order of options in the $f[opt-names] list should be required to match the order the options were defined. It seems to me you're trying to be too clever rather than using a simpler, more idiomatic, solution.

@dunsany
Copy link
Contributor Author

dunsany commented Dec 22, 2022

so far you have failed to illustrate, at least to me

Analyze the example I just gave you and try to think how the implementation of with-temp-overlay would look like and what would happen if the order was not preserved, before accusing me of being "too clever". I can agree that my "requirement" constitutes a non-standard use of the language, but it is what I'm using in my codebase currently, even if I'm "misusing options".

@hanche
Copy link
Contributor

hanche commented Dec 22, 2022

I don't really think this qualifies as "bikeshedding", though. If you feel this isn't important to you, @hanche, it's fine, but I'm not sure why you troubled yourself to communicate it here.

Sorry about the bikeshedding comment. I had first written that bit about communicating the options in the form of a map, then it occurred to me that that was getting into bikeshedding territory, so I added something about that up front without thinking too much. I believe the comment is pertinent, though. It's not so much that I personally don't care, it's more that I think there is such a thing as overspecifying the language. And moreover, that if this ordering issue becomes important to you, I can't shake the feeling that you are fighting the language. That tends to get you in trouble. But kudos for noting that you are relying on non-documented behaviour, and doing something about it! It's just that I think hat some behaviour is better left undocumented, if only to avoid tying the hands of the developers in the future.

For your particular use case, I strongly suspect that my proposal for optional arguments (#1279) would be more helpful. My original proposal was perhaps overcomplicated, but it got whittled down in the comments.

PS. Will be offline much of the time in the coming week, so don't expect much followup from me. In any case, it's the project owner you need to convince, not me.

@dunsany
Copy link
Contributor Author

dunsany commented Dec 22, 2022

my proposal for optional arguments (#1279) would be more helpful

It is a fine proposal, but it's completely unrelated to what I tried to illustrate above.


I think there is such a thing as overspecifying the language

some behaviour is better left undocumented

Language is a tool. Tools should be as predictable as possible.

There is such thing as specifying that some behavior may change in the future — in fact, it's done multiple times in the Elvish documentation already.

you are fighting the language

This whole issue isn't a feature request, it's pointing out insufficient documentation. So far none of you addressed my point that the two lists strongly indicate that the ordering of $f[opt-names] was supposed to match the definition — otherwise, why bother and not just make it a map instead?

There are at least four possibilities:

  • the ordering was intentional; or
  • the ordering was not intentional

and

  • the language intends to stick to that behavior; or
  • the language doesn't want to commit to that, it may change in the future.

My opinion is that all four possibilities warrant a comment in the documentation.

If @xiaq didn't do this intentionally, then I will try to convince him to stick to it, but it will be a separate request from clarifying the documentation.

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

No branches or pull requests

3 participants