019 integration polish

[akollegger]

No description provided.

[Copilot]

Pull request overview

This PR finalizes the Pattern library API by establishing explicit export lists, completing Haddock documentation, and implementing rigorous property-based testing. The changes represent a "polish" phase focused on API stability and documentation quality.

Key changes:

• Explicit export lists in Pattern.Core to prevent internal helper leakage
• Streamlined documentation with concise examples
• Simplified implementations for typeclass instances
• Updated CHANGELOG to reflect version 0.1.0 release

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
specs/019-integration-polish/spec.md	Defines three user stories for clean API, comprehensive documentation, and rigorous testing
specs/019-integration-polish/tasks.md	Documents completed 17-task implementation plan across 6 phases
specs/019-integration-polish/research.md	Technical decisions for API export strategy, documentation standards, and test coverage
specs/019-integration-polish/quickstart.md	User guide for verifying documentation, tests, and API exports
specs/019-integration-polish/plan.md	Implementation plan with technical context and constitution compliance checks
specs/019-integration-polish/data-model.md	Public API surface specification listing all exported types and functions
specs/019-integration-polish/contracts/api-surface.md	API contract with type signatures and laws for Pattern module
specs/019-integration-polish/checklists/requirements.md	Completed specification quality checklist
libs/pattern/src/Pattern/Core.hs	Major refactor: explicit exports, concise documentation, simplified instances, but introduces several bugs
libs/pattern/src/Pattern.hs	Added documentation structure comments for module re-exports
libs/pattern/CHANGELOG.md	Updated to reflect 0.1.0 release with feature 019
.cursor/rules/specify-rules.mdc	Added 019-integration-polish to recent changes tracking

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The allValues implementation has the same issue as anyValue. It uses a manual foldr implementation:

allValues p = foldr (\v acc -> p v && acc) True

This is less idiomatic and potentially less efficient than using the standard library's all function. The standard library version is optimized for short-circuit evaluation.

Recommend using: allValues p = all p . toList or allValues = all point-free.

Suggested change

	allValues p = foldr (\v acc -> p v && acc) True
	allValues p = all p . toList

[Copilot]

The findPattern function implementation has changed significantly. The new implementation uses a helper orElse function that is less efficient than the original.

The new code evaluates map (findPattern p) es which will traverse ALL elements even if the first one matches, then uses foldr to find the first Just value. This defeats the purpose of short-circuit evaluation.

The original implementation correctly short-circuits:

foldr (\elemPat acc -> case acc of
    Just _ -> acc  -- Already found, keep it
    Nothing -> findPattern pred elemPat) Nothing (elements pat)

However, the original logic was actually backwards - it should check elements left-to-right and stop at first match, not keep searching. The new implementation has the same flaw.

A better implementation would be:

findPattern p pat@(Pattern _ es)
  | p pat = Just pat
  | otherwise = listToMaybe $ mapMaybe (findPattern p) es

Or to truly short-circuit:

findPattern p pat@(Pattern _ es)
  | p pat = Just pat  
  | otherwise = foldr (\e acc -> acc <|> findPattern p e) Nothing es

Suggested change

	| otherwise = foldr orElse Nothing (map (findPattern p) es)
	where
	orElse (Just x) _ = Just x
	orElse Nothing y = y
	| otherwise = foldr (\e acc -> findPattern p e <|> acc) Nothing es

[Copilot]

The documentation comment states "An atomic pattern has depth 1" but the previous (removed) documentation consistently stated that atomic patterns have depth 0. This is a documentation inconsistency that conflicts with the bug in the implementation at line 942.

The documentation should be updated to match the intended behavior (depth 0 for atomic patterns) once the implementation is fixed.

Suggested change

	-- An atomic pattern has depth 1.
	-- An atomic pattern has depth 0.

[Copilot]

The depthAt function documentation and implementation are inconsistent with the depth semantics. The documentation states "distance from root + 1" but the implementation starts with depth 1 at the root:

depthAt = go 1
  where
    go d (Pattern _ es) = Pattern d (map (go (d + 1)) es)

If atomic patterns should have depth 0 (as per the original design), then depthAt should start at 0, not 1. If the new design intends depth to be 1-indexed (depth 1 for root), this represents a semantic change that should be clearly documented and consistently applied throughout.

[Copilot]

The depth function incorrectly returns 1 for atomic patterns (patterns with no elements). According to the documentation comments in the removed code (lines 0-), atomic patterns should have depth 0, not depth 1. The current implementation depth (Pattern _ []) = 1 should be depth (Pattern _ []) = 0.

This is inconsistent with:

• The removed documentation stating "Atomic patterns (no elements) have depth 0 (root only, no nesting)"
• The depth counting convention mentioned: "Depth 0: Atomic pattern (root only, no nesting levels below root)"

Suggested change

	depth (Pattern _ []) = 1
	depth (Pattern _ []) = 0

[Copilot]

The anyValue implementation has changed from using the standard library's any function with toList to a manual foldr implementation. The new implementation is:

anyValue p = foldr (\v acc -> p v || acc) False

While functionally equivalent, this is less idiomatic Haskell and less efficient because it doesn't short-circuit as effectively. The standard any function is specifically optimized for short-circuit evaluation. The current implementation will build up a chain of || operations through the entire fold even though || itself short-circuits.

Recommend reverting to: anyValue p = any p . toList or anyValue = any if using it point-free.

Suggested change

	anyValue p = foldr (\v acc -> p v || acc) False
	anyValue p = any p . toList

[Copilot]

[nitpick] Missing imports for Sum and Product that are used in the documentation examples. Lines 392, 400, and others reference Data.Monoid (Sum(..), Product(..)) in the examples, but these imports are not in the import list at the top of the file (lines 34-43).

The documentation examples use:

>>> import Data.Monoid (Sum(..))
>>> import Data.Monoid (Product(..))

However, these are shown as imports within the doctest examples rather than being available from the module's imports. While this is acceptable for doctests, it would be clearer to have these available from the main imports, especially since Data.Monoid is already imported at line 39.

[Copilot]

[nitpick] The sconcat and stimes implementations in the Semigroup instance, and mconcat in the Monoid instance use explicit type signatures within the instance declarations. While this can aid clarity, it's unconventional in Haskell to include type signatures for methods within instance declarations unless necessary for type inference.

The signatures at lines 433, 439, and 489 are redundant since the types are already determined by the instance context. Consider removing them for cleaner, more idiomatic code.

Suggested change

mconcat :: [Pattern v] -> Pattern v

[Copilot]

[nitpick] Version numbering inconsistency. The CHANGELOG shows:

• Version 0.1.0 dated 2025-11-29 with Feature 019
• Version 0.0.1 dated 2025-11-29 as "Initial release"

Both versions have the same date (2025-11-29), which suggests they were created simultaneously. This is unusual for a changelog. Typically, version 0.0.1 would have been released first, then 0.1.0 would come later with a different date.

If this is the actual first release of the library, it should probably be version 0.1.0 only, with all the work included in that version. If 0.0.1 was a previous release, it should have an earlier date.

Suggested change

	## [0.0.1] - 2025-11-29
	### Added
	- Initial release

[Copilot]

The Applicative instance implementation has been completely rewritten and the behavior appears to have changed significantly. The old implementation had special broadcasting logic for atomic patterns:

applyElements [] ys = map (pure f <*>) ys  -- broadcast function
applyElements ys [] = map (<*> pure x) ys  -- broadcast value
applyElements fs' xs' = zipWith (<*>) fs' xs'

The new implementation simply uses zipWith (<*>) fs xs without any broadcasting logic. This means:

• Old: pure (+1) <*> patternWith 5 [pure 3, pure 7] would broadcast the function to all elements
• New: The same expression would only apply to the first element (or none if fs is empty)

This is a breaking change to the Applicative semantics that may affect existing code relying on the broadcasting behavior.

Suggested change

	(Pattern f fs) <*> (Pattern x xs) = Pattern (f x) (zipWith (<*>) fs xs)
	(Pattern f fs) <*> (Pattern x xs) = Pattern (f x) (applyElements f fs x xs)
	-- | Helper for broadcasting in Applicative instance.
	applyElements :: (a -> b) -> [Pattern (a -> b)] -> a -> [Pattern a] -> [Pattern b]
	applyElements f [] x xs = map (f <$>) xs
	applyElements f fs x [] = map (<*> pure x) fs
	applyElements _ fs _ xs = zipWith (<*>) fs xs