Skip to content

Conversation

@fgm
Copy link
Owner

@fgm fgm commented May 28, 2025

Fixed #59

Also fixed the fact that orderedmap_opaque_test.go actually contained transparent tests.

@fgm fgm linked an issue May 28, 2025 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.26%. Comparing base (95d165f) to head (ce96c91).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   98.22%   98.26%   +0.04%     
==========================================
  Files          15       15              
  Lines         451      462      +11     
==========================================
+ Hits          443      454      +11     
  Misses          6        6              
  Partials        2        2              
Flag Coverage Δ
unittests 98.26% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fgm fgm force-pushed the 59-support-orderedmaprange-callbacks-mutating-the-map branch from d55be6c to 3b6608d Compare May 30, 2025 20:29
@fgm
Copy link
Owner Author

fgm commented May 30, 2025

Hi @BerniVarga Thanks for the review. I've removed the unrelated parts of the PR and merged them separately, and at this point, I'm hesitant about the better API:

  1. this version adds a new method and a composite interface, but that seems a bit irrelevant given there is only a single implementation
  2. if I add the method RangeMutable to the existing OrderedMap interface, it would break compatibility with existing code using it without implementing it. Which admittedly probably does not exist.
  3. if I add the snapshot support by default to the Slice.Range it slows it down and increases RAM usage for situations where it is not needed.
  4. If I change the function signature to take a parameter to require a snapshot, it breaks compatibility
  5. If I change the factory to add a parameter requiring snapshot support, it changes the factory signature.
  6. If I create a separate implementation adding that support it keeps compatibility but it seems a bit too much for such a small feature.

I think I'm leaning towards a variant of 5. : changing the factory signature to accept a variadic of functional options: WithStableOrdering, WithMutatorsSupport.

  • because it's a variadic, it doesn't require to be set, and supports defaults
  • some options might be relevant across multiple container types, notably Set, in which the Items iterator currently does not support mutators either.

Any suggestions ?

@fgmarand fgmarand force-pushed the 59-support-orderedmaprange-callbacks-mutating-the-map branch from 3b6608d to 4148e6a Compare June 5, 2025 12:00
@fgm fgm force-pushed the 59-support-orderedmaprange-callbacks-mutating-the-map branch from 48dae22 to 0b45ea9 Compare June 15, 2025 23:10
@fgmarand fgmarand changed the title bug(orderedmap): provide support for callbacks deleting items. feature(orderedmap): provide support for callbacks deleting items. Sep 1, 2025
@fgm fgm force-pushed the 59-support-orderedmaprange-callbacks-mutating-the-map branch from ada6697 to dc2152d Compare September 19, 2025 19:32
@fgm
Copy link
Owner Author

fgm commented Sep 30, 2025

OK, so a couple of months in, I think the way to go is:

  • either the modified 5. above.
    • But the question now becomes: "which options", because some may be incompatible with each other. Or compatible but not implemented. Consider WithMutableRange for this issue and WithStableOrdering to replace the current stable parameter. Multiplying options runs the risk of having to multiply the number of implementations.
  • or not implementing it at all. In a high-speed project in which this is being used, we actually removed the need for mutating ranges by locking them and cloning the resulting results. Since the focus of the library is performance, it might actually be the best choice.

Any suggestions ? @BerniVarga ?

@fgm fgm force-pushed the 59-support-orderedmaprange-callbacks-mutating-the-map branch from 8da35a9 to ce96c91 Compare December 1, 2025 11:02
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.

Support orderedMap.Range callbacks deleting map items

3 participants