Skip to content

Conversation

boezzz
Copy link

@boezzz boezzz commented Aug 25, 2025

refs #583

This PR introduces custom_cmp method to the Object trait. ALL the core logic (except datatime.rs) is directly copied from minijinja repo minijinja/pull/707.
Please also refer to this issue in minijinja repo for more details: mitsuhiko/minijinja#703

Changes:

  • Added custom_cmp to the Object trait

  • Integrated runtime type checking using TypeId to ensure custom_cmp is only called for objects of the matching type (Note: will cause undefined behavior when comparing objects of different types)

  • Implemented timestamp-based comparison for PyDateTime objects as an example

  • Added comprehensive tests for datetime comparison

⚠️ Known Limitation:
Comparing two different types fall back to default comparison logic and may trigger undefined bahavior. Previously, comparisons between different but related types (e.g., datetime vs. date) may return incorrect results due to a fallback to Maps comparison. modules.datetime.datetime(2025, 1, 1, 12, 0, 0)==modules.datetime.date(2025, 1, 1) would return true. This was because Both types used the default repr() implementation which returns ObjectRepr::Map. And when compared as maps with empty enumerators, the comparison logic would iterate over zero items and return true.

To address the issue above without breaking changes, we can explicitly implement repr() for custom types to return ObjectRepr::Plain.

In future PRs:

Copy link

cla-bot bot commented Aug 25, 2025

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @boezzz

@cla-bot cla-bot bot added the cla:yes label Aug 25, 2025
@boezzz boezzz marked this pull request as ready for review September 3, 2025 01:34
@boezzz boezzz requested a review from a team as a code owner September 3, 2025 01:34
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many terrible fallbacks that would become a nightmare if we started depending on. This is how languages (minijina is a programming language implementation) end up with really bad semantics. Allocating on comparisons and falling back to string comparisons is not OK.

We should think about a different solution to the specific problems on a case-by-case basis.

@felipecrv
Copy link
Contributor

Oh my. Just noticed it's an upstream change. 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants