Opened 8 years ago

Closed 8 years ago

#14248 closed enhancement (fixed)

Add case sensitive to global options

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-5.10
Component: misc Keywords: global options
Cc: sage-combinat, nthiery, andrew.mathas, novoselt Merged in: sage-5.10.beta2
Authors: Travis Scrimshaw Reviewers: Andrew Mathas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13605 Stopgaps:

Status badges

Description (last modified by 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:

Attachments (2)

trac_14248-global_options_case-review--am.patch (8.4 KB) - added by andrew.mathas 8 years ago.
Review patch which fixes some doc-strings and makes case_sensitive a boolean
trac_14248-global_options_case-ts.patch (25.0 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by tscrim

  • Keywords global options added
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by vbraun

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

comment:3 Changed 8 years ago by novoselt

  • Cc novoselt added

comment:4 Changed 8 years ago by andrew.mathas

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

comment:5 Changed 8 years ago by novoselt

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.

comment:6 follow-up: Changed 8 years ago by 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)

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

comment:7 in reply to: ↑ 6 Changed 8 years ago by andrew.mathas

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

comment:8 follow-up: Changed 8 years ago by 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.

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.

comment:9 in reply to: ↑ 8 Changed 8 years ago by andrew.mathas

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).

Last edited 8 years ago by andrew.mathas (previous) (diff)

comment:10 Changed 8 years ago by tscrim

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).

comment:11 Changed 8 years ago by andrew.mathas

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.

comment:12 Changed 8 years ago by tscrim

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.

comment:13 Changed 8 years ago by tscrim

New patch which fixes output bug in rigged_partition.py.

Last edited 8 years ago by tscrim (previous) (diff)

Changed 8 years ago by andrew.mathas

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

comment:14 Changed 8 years ago by andrew.mathas

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

Last edited 8 years ago by andrew.mathas (previous) (diff)

comment:15 follow-up: Changed 8 years ago by 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.

Best,
Travis

comment:16 in reply to: ↑ 15 Changed 8 years ago by andrew.mathas

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.

Changed 8 years ago by tscrim

comment:17 follow-up: Changed 8 years ago by tscrim

  • Description modified (diff)
  • Status changed from needs_review to positive_review

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

comment:18 in reply to: ↑ 17 Changed 8 years ago by andrew.mathas

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.

comment:19 Changed 8 years ago by tscrim

  • Description modified (diff)

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!

comment:20 Changed 8 years ago by jdemeyer

  • Reviewers set to Andrew Mathas

comment:21 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.10.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.