Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-4217

Make the glob in --show config=xxx case insensitive if "xxx" is all lower case

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_base
    • Labels:
      None
    • Story Points:
      0.25
    • Sprint:
      Science Pipelines DM-W16-2, Science Pipelines DM-W16-3
    • Team:
      Data Release Production

      Description

      See RFC-108

      The only difference is that I plan to make --show config=xxx be case-insensitive only if the pattern xxx is all lower-case. If people really want to match "background" lower-case only, they can say --show config=xxx:NOIGNORECASE

        Attachments

          Issue Links

            Activity

            Hide
            rhl Robert Lupton added a comment -

            Could you take a look at this? It's pretty short.

            Branch tickets/DM-4217 set up to track remote branch tickets/DM-4217 from origin.
            $ git diff --stat master 
             python/lsst/pipe/base/argumentParser.py | 12 ++++++++++--
             tests/testArgumentParser.py             | 12 ++++++++++++
             2 files changed, 22 insertions(+), 2 deletions(-)
            

            Show
            rhl Robert Lupton added a comment - Could you take a look at this? It's pretty short. Branch tickets/DM-4217 set up to track remote branch tickets/DM-4217 from origin. $ git diff --stat master python/lsst/pipe/base/argumentParser.py | 12 ++++++++++-- tests/testArgumentParser.py | 12 ++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-)
            Hide
            rowen Russell Owen added a comment -

            I am strongly in favor of case-blind searching, so on the whole I am very happy with this change.

            However, I dislike treating all-lowercase typing specially, as it seems too magic. And the alternative of :NOIGNORECASE is awfully clumsy, hard to type and hard to remember, though I do understand where you got it from.

            My suggestion is to NOT support case-sensitive searching at all. Period. Surely any sane search will only turn up a few matches even if case-blind?

            However, if you must support case-sensitive searching, then I would request a shorter and more obvious way to specify it. How about a suffix of ":case" (preferably ignoring case)?

            Aside from that, this looks fine to me.

            Show
            rowen Russell Owen added a comment - I am strongly in favor of case-blind searching, so on the whole I am very happy with this change. However, I dislike treating all-lowercase typing specially, as it seems too magic. And the alternative of :NOIGNORECASE is awfully clumsy, hard to type and hard to remember, though I do understand where you got it from. My suggestion is to NOT support case-sensitive searching at all. Period. Surely any sane search will only turn up a few matches even if case-blind? However, if you must support case-sensitive searching, then I would request a shorter and more obvious way to specify it. How about a suffix of ":case" (preferably ignoring case)? Aside from that, this looks fine to me.
            Hide
            rhl Robert Lupton added a comment -

            I made the change as if someone types --show config=CaseSensitiveString I think they should get what they typed. I don't expect anyone to ever want to do the case-sensitive lower case one, but you cannot just pipe to grep because of the docstrings, so I think we do need a mechanism. As it'll be very rare I think the python-inspired NOIGNORECASE is OK, but I can ask for opinions on c.l.o if you'd like.

            Show
            rhl Robert Lupton added a comment - I made the change as if someone types --show config= CaseSensitiveString I think they should get what they typed. I don't expect anyone to ever want to do the case-sensitive lower case one, but you cannot just pipe to grep because of the docstrings, so I think we do need a mechanism. As it'll be very rare I think the python-inspired NOIGNORECASE is OK, but I can ask for opinions on c.l.o if you'd like.
            Hide
            rowen Russell Owen added a comment -

            Please do ask on community.lsst.

            Show
            rowen Russell Owen added a comment - Please do ask on community.lsst.
            Show
            rowen Russell Owen added a comment - For the record, @rhl started this topic: https://community.lsst.org/t/should-we-make-all-show-config-comparisons-be-case-insensitive/344
            Hide
            rowen Russell Owen added a comment -

            One more minor comment: this

                mat = re.search(r"(.*):NOIGNORECASE$", pattern)
            

            is more simply expressed using endswith. The following is an exact replacement, given the way mat is used later in the code, but there are cleaner ways to do this:

                mat = pattern.endswith(":NOIGNORECASE")
            

            Show
            rowen Russell Owen added a comment - One more minor comment: this mat = re.search(r"(.*):NOIGNORECASE$", pattern) is more simply expressed using endswith . The following is an exact replacement, given the way mat is used later in the code, but there are cleaner ways to do this: mat = pattern.endswith(":NOIGNORECASE")
            Hide
            rhl Robert Lupton added a comment -

            Merged to master

            Show
            rhl Robert Lupton added a comment - Merged to master

              People

              Assignee:
              rhl Robert Lupton
              Reporter:
              rhl Robert Lupton
              Reviewers:
              Russell Owen
              Watchers:
              Robert Lupton, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.