Fix #11: reject quoted timestamps instead of silently parsing as null - #124
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses quoted TOML timestamps being silently treated as missing metadata, so invalid meta.toml files fail instead of producing repositories without package timestamps.
Changes:
- Adds a custom optional timestamp codec that distinguishes missing keys from type mismatches.
- Adds a regression test and fixture for quoted timestamp rejection.
- Updates executable dependencies and root-only ignore patterns for generated directories.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/Foliage/Meta.hs |
Replaces optional timestamp parsing with a custom codec that errors on invalid timestamp values. |
tests/Tests.hs |
Adds a build-failure regression test for quoted timestamps. |
tests/fixtures/bad-timestamp/_sources/pkg-a/1.0.0.0/meta.toml |
Adds an invalid quoted timestamp fixture. |
foliage.cabal |
Adds dependencies required by the custom codec implementation. |
.gitignore |
Attempts to anchor generated directory ignore patterns to the repository root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fb69a9f to
c4c0bdf
Compare
9da7c7f to
38daf05
Compare
|
Sorry, I didn't see the review request until now. I'll take a look tomorrow. |
38daf05 to
10a958d
Compare
neilmayhew
left a comment
There was a problem hiding this comment.
Nice to have this fixed!
| testCaseSteps "rejects quoted timestamp in meta.toml" $ \step -> | ||
| inTemporaryDirectoryWithFixture "tests/fixtures/bad-timestamp" $ do | ||
| step "Building repository (expecting failure)" | ||
| result <- try @SomeException (callCommand "foliage build --no-signatures") |
There was a problem hiding this comment.
| result <- try @SomeException (callCommand "foliage build --no-signatures") | |
| result <- try @IOError (callCommand "foliage build --no-signatures") |
Pedantic, I know, but SomeException always looks like a code smell to me,
Even better would be to use readProcessWithExitCode and check the exit code, and maybe even the stderr, but that's a lot of trouble for not a lot of gain.
There was a problem hiding this comment.
I forgot about auto-merge being enabled!
Oh, well, it was fine anyway.
Fixes #11.
Problem
If a user writes a quoted timestamp in
meta.toml:instead of the correct bare TOML datetime:
the old code silently parsed it as
Nothing(becauseToml.dioptionalswallows all failures, not just missing-key ones). The user ended up with a broken index — no package timestamp, brokenindex-statesupport — with no error or warning.Fix
Replaces
Toml.dioptional (timeCodec "timestamp")with a handrolledoptionalTimeCodecthat distinguishes between:Nothing(fine)The underlying
matchMaybehelper looks up the key first; if it's present it applies the BiMap and surfaces any type mismatch as an error. This is the approach discussed in the tomland issue and previously explored in #57.The error message users now see is clear and actionable:
Changes
app/Foliage/Meta.hs: addoptionalTimeCodec+matchMaybe, use insourceMetaCodecfoliage.cabal: addmtl,unordered-containers(needed by the new codec)tests/Tests.hs: new test case verifyingfoliage buildfails on a quoted timestamptests/fixtures/bad-timestamp/: minimal fixture for the new test