optionally warn when encountering tabs in whitespace by dtpowl · Pull Request #1589 · yesodweb/persistent

dtpowl · 2025-04-22T23:14:18Z

This PR's branch is based on #1584. Most of the changes in this branch are also present in that PR's branch. I strongly recommend looking at #1584 first, and I've marked this PR as a draft until that one has been reviewed.

Per a conversation with @parsonsmatt, we would like the ability to optionally emit an error or warning when the entity DSL parser encounters tab characters in whitespace. This PR adds that, with the error level (error, warning, or none) configured in PersistSettings.

To maximize compatibility with the old parser, each tab is treated as a single character of whitespace.

After submitting your PR:

Update the Changelog.md file with a link to your PR
Bumped the version number if there isn't an (unreleased) on the Changelog
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

parsonsmatt · 2025-05-15T22:25:11Z

persistent/test/Database/Persist/QuasiSpec.hs

+              let (warnings, res) = runConfiguredParser defaultParserSettings initialExtraState (some anyToken) "" s
+              case res of
+                Left peb ->
+                  (warnings, Left peb)
+                Right (tokens, _acc) -> (warnings, Right tokens)


tbh I think I would have this discard warnings for this function and another function tokenizeWithWarnings that returns warnings

This allows you to keep most of the tests the same, but then have new tests with new behavior that should have warnings

Hmm — yeah, we could do that, but I'd advocate for keeping it the way it is now, because IMO not having warnings is part of the "successful parse" behavior that the tests are intended to exercise.

i.e., if we were to in the future make a change that inadvertently caused the parse task in one of the existing tests to generate a warning, we would want that test to start failing.

parsonsmatt · 2025-05-15T22:25:49Z

persistent/Database/Persist/TH/Internal.hs

-  let cpr = parse ps s
-  case cpr of
+  let (warnings, res) = parse ps s
+  _ <- reportWarning $ renderWarnings warnings


would suggest reporting as multiple warnings

Suggested change

_ <- reportWarning $ renderWarnings warnings

_ <- traverse (reportWarning . renderWarning) warnings

parsonsmatt · 2025-05-15T22:27:16Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    ( ParserSettings (..)
+    , defaultParserSettings


We'll need to expose a safe interface for ParserSettings from Database.Persist.Quasi

Suggest creating new module Database.Persist.Quasi.ParserSettings which defines the interface + Database.Persist.Quasi.ParserSettings.Internal which exports everything

parsonsmatt · 2025-05-15T22:29:28Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+renderWarnings :: [ParserWarning] -> String
+renderWarnings warnings = intercalate "\n" $ fmap parserWarningMessage warnings
+
+-- Attempts to parse with a provided parser. If it fails with an error matching


Suggested change

-- Attempts to parse with a provided parser. If it fails with an error matching

-- | Attempts to parse with a provided parser. If it fails with an error matching

parsonsmatt · 2025-05-15T22:30:49Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    -> Parser a
+    -> Parser a
+    -> Parser a
+tryOrRegisterError msg f l r = do


If using single variable names, it is idiomatic to use p with a predicate (ie a -> Bool)

parsonsmatt · 2025-05-15T22:33:34Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    -- Due to backtracking, it is possible that we have accumulated duplicate warnings.
+    -- For example, if two parsers which depend on the same subparser both attempt to
+    -- parse the same span of input, any warnings generated by that subparser will be
+    -- registered twice.
+    filteredWarnings = Set.toList $ Set.fromList warnings


Perhaps instead of using [Warning] in the WRiterT you use Set Warning? Then you get uniqueness by construction and more efficient additions anyway

Oh yeah, of course — that's way better

I'll still convert it back to a list at this point I think? since we'll want it to be a functor later

Do we need Functor specificlaly? We can do Set.map

Is that order-preserving?

well, I guess it doesn't matter — we've thrown away the order by using Set in the first place

So I guess the right thing to do is to sort them by source position after retrieving them from the set and before presenting them

parsonsmatt · 2025-05-15T22:34:24Z

persistent/test/Database/Persist/QuasiSpec.hs

+import Database.Persist.Quasi.Internal
+import Database.Persist.Quasi.Internal.ModelParser


so these Internal imports are a bit smelly - can you make them explicit + ensure that anything we really need is available from Database.Persist.Quasi ?

So at minimum we need to import ModelParser here in order to test it. We could expose ModelParser from Database.Persist.Quasi simply in order to avoid importing Internal, but I'm a bit reluctant to do that because I can't think of any legitimate reason that a consumer of this library would need to interact with ModelParser.

It must be exported from somewhere or it can't be used in these tests, but leaving it in an Internal module is a way to signal that it shouldn't be used. The library already seems to have the pattern of exposing some things needed in tests from Internal modules; see import Database.Persist.EntityDef.Internal on line 17.

I think at minimum explicitly importing the stuff we need from Internal is a good idea, but I have a slight preference for not exposing ModelParser from Database.Persist.Quasi.

Yes - as discussed elsewhere, I'm fine importing internal modules, but I think we should try to use explicit imports and ensure that we're able to get what we need for a public API from the Database.Persist.Quasi

parsonsmatt

Code and imlpementation look great! A few comments on module organization

parsonsmatt · 2025-05-16T19:50:53Z

persistent/Database/Persist/Quasi/PersistSettings.hs

@@ -0,0 +1,91 @@
+module Database.Persist.Quasi.PersistSettings where


ok naming conventions: without Internal in the name this whole module is public API

suggest doing, like,

module Database.Persist.Quasi.PersistSettings ( public , api , goes , here ) where ... module Database.Persist.Quasi.PersistSettings.Internal where -- export everything

also love putting this in it's own module

parsonsmatt · 2025-05-16T19:52:09Z

persistent/Database/Persist/TH/Internal.hs

-  let cpr = parse ps s
-  case cpr of
+  let (warnings, res) = parse ps s
+  _ <- traverse_ (reportWarning . parserWarningMessage) $ warnings


Suggested change

_ <- traverse_ (reportWarning . parserWarningMessage) $ warnings

traverse_ (reportWarning . parserWarningMessage) $ warnings

don't need to <- out - the traverse_ function returns f () already, and you can safely not bind out of a f ()

parsonsmatt · 2025-05-16T19:54:32Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    ( Set ParserWarning
+    , Either (ParseErrorBundle String Void) (a, ExtraState)
+    )


Suggested change

( Set ParserWarning

, Either (ParseErrorBundle String Void) (a, ExtraState)

)

ParseResult (a, ExtraState)

parsonsmatt · 2025-05-16T19:55:38Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    :: (Monoid a) => [ParseResult a] -> CumulativeParseResult a
+toCumulativeParseResult prs = do
+    let
+        (warnings, eithers) = sequence prs


very nice - sequence :: Monoid a => [(a, b)] -> (a, [b])

parsonsmatt · 2025-05-16T20:01:51Z

persistent/Database/Persist/Quasi/Internal/ModelParser.hs

+    case parseEntities ps filepath (Text.unpack source) of
+        (warnings, Right blocks) -> (warnings, Right (toParsedEntityDef mSourceLoc <$> blocks))
+        (warnings, Left peb) -> (warnings, Left peb)


instance Functor ((,) r) where fmap f (r, a) = (r, f a)

We also have:

instance Functor (Either l) where fmap f (Left l) = Left l fmap f (Right r) = Right (f r)

This means we can use fmap to not bother with the tuple structure, and fmap to not bother with the Either structure:

Suggested change

case parseEntities ps filepath (Text.unpack source) of

(warnings, Right blocks) -> (warnings, Right (toParsedEntityDef mSourceLoc <$> blocks))

(warnings, Left peb) -> (warnings, Left peb)

fmap (fmap (toParsedEntityDef mSourceLoc )) <$> parseEntities ps filepath (Text.unpack source)

This is a bit golfy, but it also reduces noise.

parsonsmatt

hell yeah let's roll

parsonsmatt · 2025-05-16T20:40:26Z

persistent/Database/Persist/Quasi.hs

    , upperCaseSettings
    , lowerCaseSettings
-    -- ** Getters and Setters
-    , module Database.Persist.Quasi


hmm does this reduce the interface of the module? may be good to look at a diff of the generated haddocks. stack haddock --open for here + the haddocks on persistent hackage

Looks like the new interface is a superset of the old one, as it should be.

parsonsmatt · 2025-05-16T20:42:55Z

persistent/Database/Persist/Quasi/PersistSettings/Internal.hs

+            then parserWarningMessage l <= parserWarningMessage r
+            else warningPos l <= warningPos r
+
+-- @since 2.16.0.0


Suggested change

-- @since 2.16.0.0

-- |

--

-- @since 2.16.0.0

these need to be in haddock comment blocks i think to render out

parsonsmatt

nice 😎 let's get this in

dtpowl changed the title ~~Warnings~~ optionally warn when encountering tabs in whitespace Apr 22, 2025

dtpowl force-pushed the warnings branch from e37f533 to 25cdc4d Compare April 23, 2025 17:56

dtpowl mentioned this pull request May 7, 2025

use Megaparsec for first-pass parsing of entity definitions #1584

Merged

5 tasks

dtpowl force-pushed the warnings branch 10 times, most recently from 7eee714 to e4cc2bf Compare May 11, 2025 22:57

dtpowl force-pushed the warnings branch from 515ca9c to 6918bd2 Compare May 15, 2025 21:37

add support for optional warnings/errors + warn/error on tabs

4d8cedd

dtpowl force-pushed the warnings branch from 6918bd2 to 4d8cedd Compare May 15, 2025 22:17

parsonsmatt reviewed May 15, 2025

View reviewed changes

PR feedback

b8f7b60

dtpowl force-pushed the warnings branch from 482f476 to b8f7b60 Compare May 16, 2025 16:22

expose new setting

6c85c9f

dtpowl marked this pull request as ready for review May 16, 2025 18:28

dtpowl requested a review from parsonsmatt May 16, 2025 18:29

parsonsmatt reviewed May 16, 2025

View reviewed changes

shuffle some more stuff around between modules

7d4f1a5

dtpowl force-pushed the warnings branch from c018f9d to 7d4f1a5 Compare May 16, 2025 20:37

dtpowl requested a review from parsonsmatt May 16, 2025 20:38

parsonsmatt approved these changes May 16, 2025

View reviewed changes

fix some haddocks

0cbfa50

dtpowl force-pushed the warnings branch from d70205a to 0cbfa50 Compare May 16, 2025 20:57

Merge branch 'master' into warnings

f175ebd

parsonsmatt approved these changes May 16, 2025

View reviewed changes

add a couple of haddocks

fdbb7fc

dtpowl force-pushed the warnings branch 3 times, most recently from efc251b to 43fbd05 Compare May 17, 2025 16:33

don't require pkg-config

3ff4b66

dtpowl force-pushed the warnings branch 10 times, most recently from 1c35d82 to a18232c Compare May 19, 2025 18:49

disable qq warnings for megaparsec <9.5

7c8ae79

dtpowl force-pushed the warnings branch from a18232c to 7c8ae79 Compare May 19, 2025 18:56

QuasiSpec.specWarnings

289cf28

dtpowl force-pushed the warnings branch from 0758505 to 289cf28 Compare May 19, 2025 21:58

parsonsmatt approved these changes May 19, 2025

View reviewed changes

parsonsmatt merged commit 55e6d53 into yesodweb:master May 19, 2025
9 of 10 checks passed

	_ <- reportWarning $ renderWarnings warnings
	_ <- traverse (reportWarning . renderWarning) warnings

	-- Attempts to parse with a provided parser. If it fails with an error matching
	-- \| Attempts to parse with a provided parser. If it fails with an error matching

		import Database.Persist.Quasi.Internal
		import Database.Persist.Quasi.Internal.ModelParser

		@@ -0,0 +1,91 @@
		module Database.Persist.Quasi.PersistSettings where

	_ <- traverse_ (reportWarning . parserWarningMessage) $ warnings
	traverse_ (reportWarning . parserWarningMessage) $ warnings

Conversation

dtpowl commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtpowl May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dtpowl commented Apr 22, 2025 •

edited

Loading

dtpowl May 15, 2025 •

edited

Loading