Skip to content

Conversation

ammkrn
Copy link
Contributor

@ammkrn ammkrn commented Jun 18, 2025

Adds ASCII-only case insensitive comparison to Char and String.

@github-actions github-actions bot added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Jun 18, 2025
@ammkrn ammkrn force-pushed the ammkrn/case_ins_string branch 2 times, most recently from dc60a14 to 620dff8 Compare June 18, 2025 13:00
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 18, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jun 18, 2025

Mathlib CI status (docs):

@fgdorais
Copy link
Collaborator

I will review this in a couple of days. Regarding names, I think the core style would prefer ASCII over Ascii (though counterexamples exist, e.g Json vs JSON).

PS: If you need Unicode locale-independent case folding then please add a feature request to UnicodeBasic. I'm planning to add a few more normalization features this summer and this is an easy addition to the pile.

leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jun 21, 2025
@fgdorais
Copy link
Collaborator

fgdorais commented Jul 3, 2025

There's a lot here! A lot of which is great!

Let's split it though. Can you make a separate PR for the Char stuff only and make this PR depend on the new one?

Let me know if you need instructions or other assistance doing that.

@fgdorais fgdorais added awaiting-author Waiting for PR author to address issues and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Jul 3, 2025
@ammkrn ammkrn force-pushed the ammkrn/case_ins_string branch from 532ab26 to 848c993 Compare July 3, 2025 14:02
@ammkrn
Copy link
Contributor Author

ammkrn commented Jul 3, 2025

There's a lot here! A lot of which is great!

Let's split it though. Can you make a separate PR for the Char stuff only and make this PR depend on the new one?

I believe I was able to do this, let me know if I did something different.

Regarding names, I think the core style would prefer ASCII over Ascii (though counterexamples exist, e.g Json vs JSON).

I'll make the appropriate changes if the verdict is that ASCII is preferable.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Jul 23, 2025
@fgdorais
Copy link
Collaborator

Now that #1312 is merged, we can start working on this. This one will be much easier since we already figured out a lot in #1312. Thank you for your patience and responsiveness with the review process. I look forward to completing this PR when you're ready.

@ammkrn ammkrn force-pushed the ammkrn/case_ins_string branch from d864672 to 4aeac4e Compare July 26, 2025 11:56
@ammkrn
Copy link
Contributor Author

ammkrn commented Jul 26, 2025

I tried to match the Char setup in a way that made sense.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Jul 26, 2025
@ammkrn
Copy link
Contributor Author

ammkrn commented Jul 27, 2025

I implemented the requested changes with one modification: String.Iterator does not seem to have an instance of Stream (I was getting a 'failed to synthesize...' error). There's one for Substring which is accessible via ToStream.toStream, so I used that.

@fgdorais
Copy link
Collaborator

Looks good!

After much consideration, the quotient type CaseInsensitiveAsciiOnly is not a good fit for Batteries. There are two primary reasons:

  1. It has very limited functionality and adding more functions is tricky since it requires lifting.
  2. It is not very efficient since most functions end up applying caseFoldAsciiOnly and then another string function.

In fact, I would recommend using the subtype { s : String // s = s.caseFoldAsciiOnly } instead of the quotient type.

Please delete the CaseInsensitiveAsciiOnly file and we will be ready to merge.

@fgdorais fgdorais changed the title feat: ascii case insensitive Char/String compare feat: add case insensitive comparison for strings (ASCII only) Jul 27, 2025
@fgdorais
Copy link
Collaborator

@ammkrn Have you tried the subtype approach I suggested? I'm curious to hear whether that works for your project.

Adds ASCII-case insensitive comparisons for `String` following the
pattern established for `Char`.
@ammkrn ammkrn force-pushed the ammkrn/case_ins_string branch from 27a7816 to a982cf7 Compare August 19, 2025 13:29
@ammkrn
Copy link
Contributor Author

ammkrn commented Aug 19, 2025

@ammkrn Have you tried the subtype approach I suggested? I'm curious to hear whether that works for your project.

I have not, but I have removed the requested files and squashed the commits.

@fgdorais
Copy link
Collaborator

I fixed a tiny thing: = is better than iff for Bool expressions.

Our Mathlib check is unreliable right now. I will have to do a manual check just to be sure. I'm juggling several pots, pans and plates right now, so please ping me again if that check takes more than a couple of weeks.

Thanks for your patience!

@fgdorais fgdorais removed awaiting-author Waiting for PR author to address issues breaks-mathlib labels Aug 26, 2025
@fgdorais
Copy link
Collaborator

Local Mathlib tests have passed. Since it's unlikely that this PR would break Mathlib, this is strong enough evidence that the alleged failure is a malfunction of our CI. I am proceeding with the merge.

@fgdorais fgdorais added this pull request to the merge queue Aug 26, 2025
Merged via the queue into leanprover-community:main with commit 76408f0 Aug 26, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants