search

fmidue / logic-tasks

0 stars 1 forks source link

refactor: `genSynTree :: SynTreeConfig -> ... -> Gen (SynTree BinOp Char)` #115

Closed jvoigtlaender closed 5 months ago

jvoigtlaender commented 6 months ago

... instead of the current type taking all the config fields separately, genSynTree :: (Integer, Integer) -> Integer -> [c] -> ... -> Gen (SynTree BinOp c).

Yes, a bit of type-level information would be lost that way (the current polymorphism in c). It could be recovered by modifying data SynTreeConfig = ... usedLiterals :: String ... to data SynTreeConfig c = ... usedLiterals :: [c] ..., but it's probably not worth it.

In any case, after the refactoring, with then probably much fewer places in the code where the field names are explicitly mentioned, some of the field names of SynTreeConfig could be reconsidered/clarified. For example, usedLiterals and atLeastOccurring are not very descriptive. Possibly availableAtoms would be better for the former, and for the latter also some name mentioning what actually is occurring at least that many times.

nimec01 commented 5 months ago

Am I right that atLeastOccurring just determines the minimal amount of unique atoms to be used in the tree?

Cutout from the tests:

it "should generate a random SyntaxTree from the given parament and use as many chars as it must use" $
      forAll validBoundsSynTree $ \synTreeConfig@SynTreeConfig {..} ->
        forAll (genSynTree synTreeConfig) $ \tree -> fromIntegral (length (nubOrd (collectLeaves tree))) >= atLeastOccurring
jvoigtlaender commented 5 months ago

Yes, that’s the role of this parameter.