Skip to content

Add case_sensitive parameter to groupby() filter#1465

Merged
davidism merged 1 commit intopallets:mainfrom
wombatonfire:groupby-case-sensitive
Mar 8, 2022
Merged

Add case_sensitive parameter to groupby() filter#1465
davidism merged 1 commit intopallets:mainfrom
wombatonfire:groupby-case-sensitive

Conversation

@wombatonfire
Copy link
Copy Markdown
Contributor

This is a quick implementation test to add support for case-insensitive sorting inside the groupby() filter, intended to verify the approach.

Changes:

  • Add case_sensitive parameter to sync_do_groupby().
  • Ignore case when sorting in sync_do_groupby() if case_sensitive=False

Open questions:

  1. Is this approach correct?
  2. Should async groupby (async def do_groupby) also be changed?
  3. Should the default value for case_sensitive parameter be False for backwards compatibility, or True to align with the sort() filter behavior?

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism davidism marked this pull request as ready for review March 7, 2022 17:42
@davidism davidism force-pushed the groupby-case-sensitive branch from 27ee085 to 7d5fb45 Compare March 7, 2022 18:29
@davidism
Copy link
Copy Markdown
Member

davidism commented Mar 7, 2022

Making it the default to match other comparison filters makes sense.

This only makes the sort insensitive, not the group, which means you could end up with unexpected groups. But if you make the group insensitive as well, it makes the keys come out lower case. It's not clear what the keys should come out as (first seen case?) or how to accomplish that without reimplementing the underlying groupby function.

@davidism
Copy link
Copy Markdown
Member

davidism commented Mar 7, 2022

Yeah, I'm going to make it so the first key, whatever case that is, is returned as the key in the output.

@davidism davidism added this to the 3.1.0 milestone Mar 7, 2022
@davidism davidism force-pushed the groupby-case-sensitive branch from 7d5fb45 to 023bbba Compare March 8, 2022 14:49
Co-authored-by: Yuri Sukhov <yuri.sukhov@gmail.com>
@davidism davidism force-pushed the groupby-case-sensitive branch from 023bbba to e690f7c Compare March 8, 2022 14:55
@davidism davidism merged commit a12ad40 into pallets:main Mar 8, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add case_sensitive parameter for groupby() filter

2 participants