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

Collections: Support case-insensitive list and dictionary comparisons #4343

Closed
AllanMedeiros opened this issue May 11, 2022 · 14 comments
Closed
Labels
acknowledge To be acknowledged in release notes alpha 2 enhancement good first issue Good for newcomers priority: medium
Milestone

Comments

@AllanMedeiros
Copy link

When comparing Lists or Dictionaries would be great if we could ignore case on item, value or both.
Something similar was done in the past with other kws: #2439
This would apply to these kws:
Dictionaries Should Be Equal
Dictionary Should Contain Item
Dictionary Should Contain Key
Dictionary Should Contain Sub Dictionary
Dictionary Should Contain Value
Dictionary Should Not Contain Key
Dictionary Should Not Contain Value

And List kws:
List Should Contain Sub List
List Should Contain Value
List Should Not Contain Duplicates
List Should Not Contain Value
Lists Should Be Equal

@pekkaklarck
Copy link
Member

Sounds good to me. Are you @AllanMedeiros, or someone else, interested to create a PR? Should be pretty straightforward. The only design decision I see is deciding should with dicts this be applied both to keys and values.

@AllanMedeiros
Copy link
Author

Could something like be ignore_case= [key | value | both] ?

@pekkaklarck
Copy link
Member

Options key, value and both sound good to me. Probably should also support True and I guess it should be same as both.

@sunday2
Copy link
Contributor

sunday2 commented Aug 13, 2022

Ignore case onlly used for the string type, but in fact, there may be other types of elements in list and dict collection. I.e., list, dict, number, dict inside list, list inside list, etc.

@pekkaklarck
Copy link
Member

If the ignore case functionality would be enabled, we obviously should ignore it with non-string values. It should probably be used with nested lists/dicts, though, but that that makes the implementation a bit more complicated.

@MobyNL
Copy link
Contributor

MobyNL commented Mar 14, 2023

Hey, I've been playing around a bit and think I have the ignore_case working for both lists and dictionaries at the moment. It also works for non-string values, so it can also work when a nested list or dict is encountered.

I tried to make it as clear as possible, but can imagine that what's clear to me is not clear to everyone. Since this is my first time contributing to something like this I would love to get feedback. I think I also have not be upholding the some standard coding principles (same for the atests). But while I work on that I would like to have some feedback already. If entire sections need to be deleted, I don't have to rework them (to compy with the standards). And I also need to add some documentation about the various ways to use ignore case, as it differs somewhat between keywords.

@MobyNL
Copy link
Contributor

MobyNL commented Mar 31, 2023

I updated the changes in #4690 . Please let me know if there are any more remarks, and whether I should change this from a draft pull request to a regular one

Abhilash-du pushed a commit to Abhilash-du/robotframework that referenced this issue Oct 12, 2023
This commit introduces a new `ignore_case` option to the dictionary comparison
methods. When `ignore_case` is set to `True`, the comparison will be
case-insensitive for both keys and values.

Closes robotframework#4343
@Abhilash-du
Copy link

Hello,
I wanted to let you know that I've taken action on the feature request mentioned in issue #4343 . I've created a pull request that addresses this request by adding the option to ignore case in Dictionary and List comparison keywords.

This is my first contribution to the project, and I'm excited to be a part of the community.

You can view the details and changes in the pull request here: #4897

Your feedback and thoughts on this would be greatly appreciated. I believe this enhancement could benefit the community by making Dictionary and List comparisons more versatile.

Thank you for your valuable suggestion, and I look forward to your feedback.

@pekkaklarck
Copy link
Member

Based on a quick look the PR #4690 by @MobyNL looks to be in so good shape after the latest changes that I'll add this issue into RF 7.0 scope. I won't have time to re-review the PR properly right now, but setting the milestone makes sure it won't be forgotten. If you @MobyNL have time, you can update the "New in 6.2" notes to correct version.

Because the aforementioned PR is in such a good shape, I closed the newer PR #4897 by @Abhilash-du. I'm sorry for letting the older PR and this issue be open so long that there was confusion about the status. I hope you find something else you can contribute to related to Robot Framework or tool in its ecosystem.

@pekkaklarck pekkaklarck added this to the v7.0 milestone Nov 2, 2023
@MobyNL
Copy link
Contributor

MobyNL commented Nov 3, 2023

I'll take a look at it this weekend 👍

@pekkaklarck pekkaklarck added the acknowledge To be acknowledged in release notes label Nov 17, 2023
@pekkaklarck
Copy link
Member

This was implemented in PR #4690 by @MobyNL. In practice various keywords got new ignore_case argument. Big thanks for contribution!

I was doing small cleanup to the new code when I noticed that the library already has some keywords that accept case_insensitive and whitespace_insenstive arguments. I had forgotten about that, but Git history showed they were added already in RF 2.8.8 (#1724), Having both ignore_case and case_insensitive in the same library is very confusing so we need to do something to it. It would be easy to change the newly added ignore_case to case_insensitive, but we also use ignore_case widely in BuiltIn and having consistency between standard libraries is important as well.

The keywords in Collections already have case_insensitive are:

  • should_contain_match
  • should_not_contain_match
  • get_matches
  • get_match_count

Because there are only for keywords, we could pretty easily also add ignore_case to them and deprecate case_insensitive. That is more work, but I believe consistency is worth it. For full consistency we should also look at the String library that has two keywords accepting case_insensitive as well. We may not have time to update that library now, but we should at least submit an issue about doing it later.

I need to think a bit more about this, but I believe using ignore_case consistently is the way to go. I'll reopen this issue until the current inconsistency is resolved somehow.

@pekkaklarck pekkaklarck reopened this Nov 17, 2023
@pekkaklarck pekkaklarck changed the title Feature request: add option to ignore case on Dictionary and Lists comparison keywords Collections. Support case-insensitive comparisons Nov 17, 2023
@pekkaklarck
Copy link
Member

While doing small cleanup to Collections related to this enhancement and otherwise, I noticed that case-insensitive comparisons with dictionaries had some problems. After a bit of investigation it turned out that it is actually pretty complicated problem, especially when we wanted to allow normalizing either only keys and values in addition to normalizing the whole dictionary. Some examples about complexity below:

  1. If a dictionary has multiple keys that are normalized to the same value like {'a': 1, 'A': 2}, normalizing removes values. We noticed this already when working with PR Feature/4343 add ignore case to list and dictionary comparisons #4690 and decided to make this an error. There was, however, a problem that this was only an error with Dictionaries Should Be Equal, but also other dictionary comparison keywords are affected.
  2. If normalizing only keys or values, the expected values need to be normalized according to the same normalization configuration as the dictionary. This requires having special methods for normalizing keys and values.
  3. With nested dictionaries, key/value normalization should be taken into account also with child dicts. For example, if only values are normalized with {'A': 'X', 'nested': {'B': 'Y'}}, both X and Y should be normalized but A and B should not. B is part of the value of the item nested, but it is also a key so it shouldn't be normalized.

Getting all the above right wasn't exactly trivial, but after the changes in the above commit everything not ought to work as expected. As part the changes I needed to enhance the Normalizer helper class that was added earlier quite a bit. I enhanced it also to handle sorting with lists and ignoring keys with dicts and now also they work recursively (#4952).

I have now also handled the inconsistency with ignore_case and case_insensitive so that old keywords using the latter now also support the former (#4954). Now it is possible to use ignore_case consistently with all standard libraries.

This enhancement is very useful, it showed the inconsistency we had with configuring case-insensitivity, and the new Normalizer class made it easy to enhance Collections also otherwise. All this required lot of work, though, which was a bit unfortunate because we have somewhat tight schedule with RF 7.0. Anyway, now everything hopefully is ready and we can enjoy the new enhancements. I hope people interested in this can test the functionality when RF 7.0 alpha 2 is out.

@pekkaklarck pekkaklarck changed the title Collections. Support case-insensitive comparisons Collections: Support case-insensitive list and dictionary comparisons Nov 22, 2023
@AllanMedeiros
Copy link
Author

This is really awesome! Can't wait to be able to test it, but to be honest I didn't realize it would require that lot of work. This happens all the time on IT world, it doesn't? A feature that seems to be simple to be implemented at first sight and then becomes complex during the development phase.
Thank you @pekkaklarck !

@pekkaklarck
Copy link
Member

Yeah, it's pretty common that something that initially looks simple turns out to be surprisingly complicated. Case-insensitivity with dictionaries was a very good example.

You can test the functionality with RF 7.0 alpha 2 that was released few days ago:
https://github.com/robotframework/robotframework/blob/master/doc/releasenotes/rf-7.0a2.rst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge To be acknowledged in release notes alpha 2 enhancement good first issue Good for newcomers priority: medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants