Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Feb 10, 2026

Abstracts the toMatching and matchCdOdSolution functions to work with variable numbers of class and object diagrams, instead of being hard-coded to 2 CDs and 5 ODs.

Changes Made

  • toMatching: Now takes a list of CD keys [Int] as a parameter instead of hard-coding [1, 2]. Uses M.toList m to iterate directly over key-value pairs, avoiding repeated M.lookup calls and the Maybe wrapper for improved efficiency.

  • matchCdOdSolution: Now derives CD keys from the instance's diagrams field using M.map (const []) (diagrams task) instead of hard-coding [(1, []), (2, [])].

  • Call site: Updated matchCdOdEvaluation to pass M.keys $ diagrams task (the actual CD keys) to toMatching.

These changes ensure the evaluation and solution logic correctly handles instances with different numbers of diagrams while improving performance by eliminating repeated map lookups.

Testing

  • ✅ Code compiles successfully
  • ✅ All targeted tests pass (10 examples, 0 failures)
  • ✅ EditorConfig compliance verified
  • ✅ HLint shows no issues

The changes maintain clean, idiomatic Haskell code while properly abstracting over the number of diagrams and improving efficiency.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copilot AI requested a review from jvoigtlaender February 10, 2026 09:05
Copilot stopped work on behalf of jvoigtlaender due to an error February 10, 2026 09:05
@jvoigtlaender
Copy link
Member

@copilot: For the cds in toMatching, the derivation from M.elems m can be wrong sometimes. Instead, abstract that function explicitly over the original number 2 (now making this a parameter of the function).

…chCdOdSolution

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 10, 2026

@copilot: For the cds in toMatching, the derivation from M.elems m can be wrong sometimes. Instead, abstract that function explicitly over the original number 2 (now making this a parameter of...

Changed toMatching to take an explicit Int parameter for the number of CDs, using take numberOfCds [1 ..] instead of deriving from M.elems m. Also simplified matchCdOdSolution to use M.map (const []) (diagrams task). Commit: 8bb4c62

Copilot AI changed the title [WIP] Update support for variable amount of class and object diagrams Abstract toMatching and matchCdOdSolution over hardcoded CD/OD counts Feb 10, 2026
Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copilot AI requested a review from jvoigtlaender February 10, 2026 09:31
@jvoigtlaender jvoigtlaender marked this pull request as ready for review February 10, 2026 09:33
Copilot AI review requested due to automatic review settings February 10, 2026 09:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the MatchCdOd task’s evaluation/solution logic to work with the actual set of class-diagram keys in the instance, instead of assuming exactly two CDs (and a fixed OD set), making the code resilient to instances with variable diagram counts.

Changes:

  • Generalized toMatching to accept the list of CD keys and to iterate via M.toList over OD mappings.
  • Updated matchCdOdEvaluation to pass M.keys (diagrams task) into toMatching.
  • Updated matchCdOdSolution to derive CD keys from diagrams task instead of hard-coding [(1, []), (2, [])].

Comment on lines +243 to +245
toMatching :: [Int] -> Map Char [Int] -> Map (Int, Char) Bool
toMatching cds m =
M.fromList [((cd, od), cd `elem` cdList) | cd <- cds, (od, cdList) <- M.toList m]
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The new abstraction for variable CD keys/OD keys (in toMatching and matchCdOdSolution) isn’t covered by the existing MatchCdOdSpec tests (they mostly cover instance generation and getODInstances). Consider adding a unit test that constructs a MatchCdOdInstance with non-default diagrams keys (e.g., 3+ CDs and/or non-1/2 keys) and asserts matchCdOdSolution/toMatching produce the expected mappings.

Copilot uses AI. Check for mistakes.
matchCdOdSolution task = M.toList $ reverseMapping (fst <$> instances task)
where
reverseMapping :: Map Char [Int] -> Map Int Letters
reverseMapping = fmap (fmap Letters) . M.foldrWithKey
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

reverseMapping = fmap (fmap Letters) ... looks ill-typed / incorrect here: M.foldrWithKey builds a Map Int String (via M.adjust (x:) onto []), so you only need to wrap each String once (e.g., fmap Letters/M.map Letters). The extra inner fmap tries to map Letters over the characters of the String and should not typecheck / would produce the wrong shape.

Suggested change
reverseMapping = fmap (fmap Letters) . M.foldrWithKey
reverseMapping = fmap Letters . M.foldrWithKey

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

If that problem were real, it would show up as a compiler error ...

@jvoigtlaender jvoigtlaender merged commit b72edfa into number-words Feb 10, 2026
27 checks passed
@jvoigtlaender jvoigtlaender deleted the copilot/sub-pr-594 branch February 10, 2026 09:45
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.

2 participants