Add case sensitive to global options

[tscrim]

Currently all string values are converted to lowercase in global options, so checks are case insensitive. This will add an option to allow case distinctions.

Apply:

• attachment: trac_14248-global_options_case-ts.patch

Depends on #13605

CC: @sagetrac-sage-combinat @nthiery @AndrewAtLarge @novoselt

Component: misc

Keywords: global options

Author: Travis Scrimshaw

Reviewer: Andrew Mathas

Merged: sage-5.10.beta2

Issue created by migration from https://trac.sagemath.org/ticket/14248

[tscrim]

Changed keywords from none to global options

[vbraun]

comment:2

For all the non-Americans out there: "Entree" is the name for the main course in `Murrica

[AndrewMathas]

comment:4

Hi Travis,

Sorry, have been meaning to review this for a while as well...

I just looked over the code and values_case doesn't seem very meaningful to me. I think that it would be better to have optional argument case_sensitive which is either True or False, with whatever default you think reasonable. 

I guess with your patch you allow the options to be forced to be either upper case, lower case, or to be case sensitive. Perhaps this is better than simply case sensitive or insensitive. Given that I was happy to always enforce lower case I am perhaps not the best person to comment on what is best here!

Will be happy to finalise a review once some one else comments on this.

Andrew

[novoselt]

comment:5

I like case_sensitive=True/False more! I don't really see the point if forcing the user to use either lower or upper case, while forcing/not forcing exact replication of the "original" makes sense.

I also think that in automatically generated documentation things should show as entered no matter whether the case is forced or not, i.e. if I have in my code "English", it should show in documentation as "English" even if we accept "english" as well.

[tscrim]

comment:6

My original thought on giving both options was in case someone wanted to force lower/uppercase display. Although this ticket came about because of #2023 in which I needed to have the ability to leave it as uppercase. Instead what we could do is have a second parameter display_type which could take one of the following values:

• uppercase
• lowercase
• input - the exact user's input
• value - the exact value given in the code (assuming we have a set list of possible values)

So for things like convention, we'd use value since we'd want English and French to display and to turn off case sensitivity for checking input. Does this sound like a better solution?

Actually I wonder if we should put something in the autogen doc about case sensitivity. On that note about autogen doc, it should not reflect the code, but instead the displayed value. That is what the user sees, and it would be more trouble explaining why the doc says "English" while the outputted option is always "english" IMO. (My principle is that if you're coding it, you "know" what you're doing, but typically the user does not.)

Best,

Travis

[AndrewMathas]

comment:7

Replying to @tscrim:

My original thought on giving both options was in case someone wanted to force lower/uppercase display. Although this ticket came about because of #2023 in which I needed to have the ability to leave it as uppercase. Instead what we could do is have a second parameter display_type which could take one of the following values: - ** uppercase** - ** lowercase** - ** input** - the exact user's input - ** value** - the exact value given in the code (assuming we have a set list of possible values)

I think that we should keep it as simple as possible: using two parameters to control this strikes me as being a little OTT. In light of Andrey's comments I vote for having case_sensitive=True/False and also following Travis's suggestion that the case of the default value is preserved in the documentation. I think that this is the most intuitive approach and that it also meets Travis' requirements for #2023. In terms of the code, it is easy to make it appear to the user that the default version of the option, together with its supplied choice of case, is always being used even though the option may not be case sensitive.

Btw, I am fairly sure that the documentation already does mention that all options are forced to be lower case. When I do the review I'll go through and fix up some of the many typos that are there and also make sure that this is addressed.

Andrew

[nbruin]

comment:8

Would it be too painful to make "keep as is" the default? All of python, and in particular string matching, is normally fully case sensitive. It feels very strange that this is not, even if it might be considered convenient by some.

In particular, by the time someone is setting options he/she is a stage beyond "beginner" and they'll have to look up the name of the option anyway, so I don't think in practice the convenience will really matter.

In most of python, ambiguity about case is removed by preferring all-lower-case. (I know, singletons True, False, None oddly deviate from that)

Incidentally, the need for options has a code smell to me: I'd start to look for a design flaw if I felt the need to introduce one.

[AndrewMathas]

comment:9

Replying to @nbruin:

Would it be too painful to make "keep as is" the default? All of python, and in particular string matching, is normally fully case sensitive. It feels very strange that this is not, even if it might be considered convenient by some.

No, it would not be painful. In fact this is what is being suggested above.

In particular, by the time someone is setting options he/she is a stage beyond "beginner" and they'll have to look up the name of the option anyway, so I don't think in practice the convenience will really matter.

In most of python, ambiguity about case is removed by preferring all-lower-case. (I know, singletons True, False, None oddly deviate from that)

This is what is currently done.

Incidentally, the need for options has a code smell to me: I'd start to look for a design flaw if I felt the need to introduce one.

Disagree on this one - I guess you don't use bashrc files etc?

I think that options are useful for either reflecting user preferences (for example, changing how something is displayed, often corresponding to different conventions in the literature), or for controlling parameters to a class (for example, see the options used in #13131 - which isn't using GlobalOptions, but could).

[tscrim]

comment:10

Here's a new version of the patch which gives the option for case sensitivity as per the discussion.

The implementation (if anyone cares) is easily extendable to what I was originally suggesting if it is decided to go that route (also this was the easiest way I could think of to do the implementation).

[AndrewMathas]

comment:11

I notice the patch makes a few of the options of partitions and tableaux case sensitive but to me it is not obvious why some are and some aren't. Could you please explain the rationale for this?

Certainly having some options being case sensitive and others not in a class of options is potentially confusing...perhaps case sensitivity should apply to all of the options so that they are either all case sensitive or all not.

[tscrim]

comment:12

The reason why the options for partitions are not case sensitive is because they are tied to the output (and they don't have a set of possible values). You don't want to be outputting x, then try to change it to X, but not see a difference. I would expect it to change.

I don't want all options to be case (in)sensitive for this reason.

[tscrim]

comment:13

New patch which fixes output bug in rigged_partition.py.

[AndrewMathas]

Review patch which fixes some doc-strings and makes case_sensitive a boolean

[AndrewMathas]

comment:14

Attachment: trac_14248-global_options_case-review--am.patch.gz

Hi Travis,

I finally got around to reviewing this. The attached review patch, which is also in the queue, fixes number of doc-string problems (most of which predate your patch) and makes case_sensitive into a proper boolean. The patch is also in the combinat queue.

Please have a look at the review patch. If you're happy with this then I'll make this a positive review.

Andrew

[tscrim]

comment:15

Hey Andrew,

Thank you for doing the reviews. However I think these should be case-sensitive by default, and so line 777 should be

self._case_sensitive[option] = True    # ``True`` by default

This would also match with the documentation and typical (python) code semantics.

Best,

Travis

[AndrewMathas]

comment:16

Replying to @tscrim:

Hey Andrew,

Thank you for doing the reviews. However I think these should be case-sensitive by default, and so line 777 should be

self._case_sensitive[option] = True    # ``True`` by default

This would also match with the documentation and typical (python) code semantics.

Yes, sorry, I agree.

[tscrim]

Attachment: trac_14248-global_options_case-ts.patch.gz

[tscrim]

comment:17

Hey Andrew,

I've uploaded the patch which has your review patch folded in and makes the minor tweak above. I'm setting this to positive review since we're in agreement about the patch (please correct me if I'm wrong).

Thank you again for reviewing this,

Travis

For patchbot:

Apply: trac_14249-global_options_case-ts.patch

[AndrewMathas]

comment:18

Replying to @tscrim:

Hey Andrew,

I've uploaded the patch which has your review patch folded in and makes the minor tweak above. I'm setting this to positive review since we're in agreement about the patch (please correct me if I'm wrong).

Yes, that's good: it should be a positive review. I didn't set it to a positive review because I noticed that the review patch, with the proper defaults, wasn't yet on trac.

[tscrim]

comment:19

I'm not able to overwrite files the you've uploaded and wasn't sure if you were going to do it in the review patch or not. Anyways...we're done. Yay!

[jdemeyer]

Reviewer: Andrew Mathas

[jdemeyer]

Merged: sage-5.10.beta2