Builtin: Add case-insensitivity support to comparison keywords

[HelioGuilherme66]

For reference see topic on google group.

(...) Perhaps it could work like this:

Should Be Equal    ${string}    expected    ignore_case=True
Should Contain    ${list}    expected    ignore_case=True

• Must be backward compatible (default value as before)
• Must have Unit Tests expanded for new option.

---

Update by @pekkaklarck:

These keywords should get the new ignore_case argument:

• Should (Not) Be Equal
• Should (Not) Be Equal As Strings
• Should (Not) Start With
• Should (Not) End With
• Should (Not) Contain
• Should Contain X Times
• Should (Not) Contain Any (added in RF 3.0.1, see New Should Contain Any and Should Not Contain Any keywords #2120)
• Should (Not) Match

[pekkaklarck]

Yes, this would be nice. If someone is interested to implement this, I'm willing to review PRs. This shouldn't cause backwards compatibility issues so the enhancement can be included to any release. The first step would be listing affected keywords in BuiltIn, String, and possibly in other libraries.

Few things to take into account:

• I'd prefer acceptance tests over unit tests
• Need to decide what is the name of the new option. ignore_case is my current favorite, but could also consider e.g. case_insensitive (longer) and caseless (shorter but not real word).
• What to do if value is not a string? I think it would be best to leave the value as-is.

[chriscallan]

Doesn't look like it would be too difficult to implement this, although it could be a death by 1000 cuts if implemented everywhere that a string "could" be used.
It would seem that adding a case insensitivity option to generic methods like "Should Be Equal" may be a bit error prone with the assumptions about the values sent in. I think that adding the case sensitivity option to only "Should Be Equal As Strings" is a more logical, and limited, way to implement this functionality while limiting the assumptions about values being passed in.
Besides, hasn't the consumer of this keyword already declared that they want to compare as strings by indicating they want to do a case-insensitive comparison? :-)
I'd be happy to take this on, preferably the more limited approach I've suggested.

[pekkaklarck]

I agree adding this functionality to each and every keyword possibly operating with strings can be too big a task. Better to add this to the most important keywords only and then add it to new keywords later if needed. That said, I think there are quite many important keywords that ought to get this functionality from the beginning:

• Should Be Equal (this is the most common comparison keyword and most often used with strings)
• Should Be Equal As Strings
• Should Contain
• Should Match
• Should Match Regexp (should probably compile the pattern with re.IGNORECASE option in this case)

These would be nice too:

• Should Start With
• Should End With

Also the "Not" versions of the above keywords (e.g. "Should Not Be Equal") should probably support this. Any other keyword people think are important?

Although I'd like to see all these added together in one release, I'd prefer them to be implemented as separate PRs. That would split the implementation workload and also make reviewing easier. Probably it would be best to have a PR for each keyword and its "Not" version.

[chriscallan]

Glad to see we're on the same page. That was the same list of keywords that I came up with during my research, and it was short enough to not seem overly daunting.
I'll get started on this today and should have something for you to review in the very near future.

[pekkaklarck]

Awesome! Please submit a PR with only one or two kws first. If there are things to fix, you can then avoid those issues in latter PRs.

[chriscallan]

I'm having some troubles with the pull request for the first two keywords. Getting "403 Permission Denied" messages when doing the push, or trying to.
I haven't done any Pull Requests through github before, so not sure if there are any permissions that need to be set or if I'm just doing something wrong :-)
Any ideas on what the issue might be here?

[pekkaklarck]

Did you look at CONTRIBUTING.rst? In addition to project specific stuff, it has links to GitHub docs about PRs etc. The highlevel workflow is like this:

1. Create your own fork and clone it locally.
2. Make a topic branch for these changes. Commit changes to the branch and push them to GitHub.
3. Create a PR based on the branch on GitHub.

[chriscallan]

Thanks for pointing me in the right direction :-) Working with GitHub projects is a bit different than I'm used to.
I think I've got the pull request setup properly and that I've got the acceptance test cases worked out.
Just the "Should Be Equal", "Should Be Equal As Strings" keywords.

[pekkaklarck]

Yeah, GitHub and Git have their learning curves (I'm definitely not a master myself!) but they are so ubiquitous that learning them helps also with other projects. The PR #2447 seems to be setup just fine and also looks pretty good. There were some smallish things I hope you could fix, though, and I added comments to the PR itself. Thanks for your work so far and let me know there or here if you have any questions about the comments!

[chriscallan]

Not sure if you saw my latest PR for this. Includes the rest of the keywords and acceptance test cases. Didn't want to ping you via message in case you were already alerted when PR's are submitted.

[pekkaklarck]

Hi, I saw updates but been busy with a training trip. I ought to have time to concentrate on RF 3.0.1 more next week. Why did you close PR #2447? Are you planning to create a new one?

[chriscallan]

I had closed PR #2447 and opened another because it seemed easier to close that one down, make the additional changes and start a fresh PR. I opened #2450 with the entire set of changes.

[pekkaklarck]

Having all changes in one PR (#2450) turned out to be a bad idea, but @chriscallan has now opened #2461 about Should (Not) Be Equal only. The PR is in very good shape and pretty much ready to be merged, but we still need to decide how do we report failures when case_insensitive=True. I originally commented that in the review comments to #2461, but wanted to move the discussion here because others may also want to comment about the design.

Currently the failure you get when you use

Should Not Be Equal    Foo    FOO    case_insensitive=True

is foo == foo. The alternative would be showing the original values with a note about case-insensitivity like Foo == FOO (case-insensitively). With Should Not Be Equal both are OK, but the latter would probably be a bit more informative.

The main problem with showing the normalized versions is that when using Should (Not) Contain with a container like a dictionary, converting the container to lowercase so that all original information is preserved in the error message would be hard. I'm currently thinking the implementation could be something like:

if is_truthy(case_insensitive) and is_string(item):
    item = item.lower()
    if is_string(container):
        container = container.lower()
    if is_list_like(container):
         # this loses information by turning tuples, dicts, sets, etc. into lists
        container = [c.lower() for c in container]   

If we want to show the normalized versions in this case, we needed to inspect the container type and try to preserve it along with possible additional data like dictionary values. It would be easier to just show the original values in the failure message in this case. I think all keywords should work same way, and thus also others should show the originals and not the normalized versions.

What do others think about this? I think implementation wouldn't be too complicated and could be done in the helper method creating the failure message.

[pekkaklarck]

Decided not to add this support to Should (Not) Match Regexp. They already support embedding case-insensitivity flag into the pattern. If we add explicit case-insensitivity support, should consider adding support also for other regexp flags.

[pekkaklarck]

All the keywords listed in the original description now have got ignore_case=False option. I'm not entirely happy about error reporting (see my comment above), but don't have time to enhance it now. I'll submit a separate issue about that for RF 3.1.

[pekkaklarck]

Enhancing error reporting is #2512. This issue can be considered done.