basho / cuttlefish

never lose your childlike sense of wonder baby cuttlefish, promise me?
Apache License 2.0
205 stars 124 forks source link

Use ordsets in apply_mappings() fold #160

Closed uwiger closed 9 years ago

uwiger commented 9 years ago

The fold function in apply_mappings() would simply cons mappings to lists for either dropping or keeping a mapping, and then subtracting the two. The effect was, in a sense, a majority vote: if more mappings than not indicated dropping, the translation would be dropped, counter-intuitively.

Solution: make the accumulators ordsets instead. Two test cases were added: one with 1 'good mapping' and 1 'bad', and the other with 1 'good' and 2 'bad'. The second test failed before the fix:

uwair:cuttlefish uwiger$ rebar eunit skip_deps=true suites=cuttlefish_generator
…
  cuttlefish_generator: apply_mappings_translations_dropped_correctly_mixed2_test...*failed*
in function cuttlefish_generator:'-apply_mappings_translations_dropped_correctly_mixed2_test/0-fun-2-'/2 (src/cuttlefish_generator.erl, line 948)
in call from cuttlefish_generator:apply_mappings_translations_dropped_correctly_mixed2_test/0 (src/cuttlefish_generator.erl, line 948)
**error:{assertEqual_failed,[{module,cuttlefish_generator},
                     {line,948},
                     {expression,"TranslationsToDrop"},
                     {expected,[]},
                     {value,["mapping.name"]}]}
=======================================================
  Failed: 1.  Skipped: 0.  Passed: 23.

After the fix:

uwair:cuttlefish uwiger$ rebar eunit skip_deps=true suites=cuttlefish_generator
…
=======================================================
  All 24 tests passed.
seancribbs commented 9 years ago

Disregard my previous comment, I understand now, I think. If this is the case, why not use ordsets:subtract/2 at the bottom of that function instead of --?

uwiger commented 9 years ago

You could, of course, but since the ordsets module has a defined representation (ordered lists with no duplicates), I felt that the line using '--' could remain. But it's your code - you decide. :)

seancribbs commented 9 years ago

It's just a nitpick, not significant.

seancribbs commented 9 years ago

:+1: 8fec121

seancribbs commented 9 years ago

@borshop merge