search

TedDriggs / darling

A Rust proc-macro attribute parser
MIT License
983 stars 66 forks source link

feat: Add `FromMeta` support for `BTreeMap` #287

Closed Techassi closed 1 month ago

Techassi commented 5 months ago

This PR adds an FromMeta implementation for BTreeMap<String, V> and BTreeMap<syn::Ident, V> by adjusting the existing macro. The macro can now handle both HashMap and BTreeMap, because much of the underlying code is the same.

I also added new unit tests to test the BTreeMap implementation.

TedDriggs commented 5 months ago

The code for this looks fine to me, but I am curious what the use-case is that needs this. Are you doing something that needs the keys to always be sorted?

Techassi commented 4 months ago

Yeah I had a very rare use-case where this behaviour was required. But I can see that this feature seems to be needed very rarely.

TedDriggs commented 4 months ago

@Techassi is this something you'd still use if it was merged? If so, can you elaborate on the use-case?

Techassi commented 3 months ago

One specific use-case was the definition of versions (like SemVer or Kubernetes API versions), which would enforce proper ordering via (Partial)Ord.

But I can see that this feature would be used very rarely, so feel free to close this PR if it introduces too much complexity.

Veetaha commented 2 months ago

I think this feature would be rather useful. BTreeMap is underrated. The main thing is that sorting guarantee gives stability. I personally prefer BTree data structures due to that. It helps guarding against flaky bugs. One case from production for me a case where code was taking the first element from a map and deciding which DB index to use based on that element. By using a HashMap that decision was always doing a random choice of an element from the map (due to unspecified ordering in the hash map) and thus introduced a flaky bug that reproduced randomly. I believe same concerns apply anywhere, where the iteration order of the map may influence the behavior of the program, including a proc macro. It may not be required by the business logic, but may be a good safety precaution against flaky bugs.

So having a Btree variant of data structure support is a win, and would be cool to get this merged.

TedDriggs commented 2 months ago

I will review and merge next week.