Add position info to EntityDef #1569

lf- · 2025-02-12T00:42:37Z

Internal Mercury bug: DUX-3027
Fixes: #1567

This is an initial implementation that we can API-compatibly improve in
the future by providing more accurate information. However, to do that,
we would have to rewrite the entity definition parser with e.g.
megaparsec. This is a reasonable course of action but rewriting it is
going to be more work and we can ship position info we could improve
later with the existing parser.

Currently this gives users the entire file in which the model is defined, but not anything closer since the parser throws all that stuff out.

Before submitting your PR, check that you've:

Documented new APIs with Haddock markup
Added @since declarations to the Haddock
Ran stylish-haskell on any changed files.
Adhered to the code style (see the .editorconfig file for details)

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)

lf- · 2025-02-12T00:47:43Z

idk if i parsed the PVP correctly here, this might actually be non breaking so perhaps should be the C instead of B component, but it will blow up anyone using the internals of the quasiquoter/TH stuff.

parsonsmatt

nice! a couple suggestions. don't need to worry about the version bump on persistent-test, once we release I'll revise the Hackage page and/or we can do a patch version bump

parsonsmatt · 2025-02-20T22:59:39Z

persistent-test/ChangeLog.md

+* [#1569](https://github.com/yesodweb/persistent/pull/1569)
+  * Version bounds for Persistent 2.15.0.0, also synchronize version
+    number with `persistent`.


We don't need to worry about this sync. If it's just version bounds then it can be a patch bump or even an (unreleased) and I'll make a revision on Hackage

parsonsmatt · 2025-02-20T23:01:44Z

persistent/Database/Persist/TH.hs

 --
 -- @since 2.5.3
-parseReferences :: PersistSettings -> Text -> Q Exp
+parseReferences :: PersistSettings -> [(Maybe SourceLoc, Text)] -> Q Exp


Ah, dang, this is listed as -- * Internal but is not itself part of an .Internal module. So this is a breaking change and we need 2.15 for this.

oof! well i suppose i guessed right at least?

We probably should move it to a .Internal module though.

Okay I just committed moving it. Feel free to review commit-by-commit and maybe clone locally and use git diff --color-moved if that's useful.

We can drop that commit if we want, it wasn't hard to write but it does prevent this happening in the future.

persistent/Database/Persist/Types/Base.hs

parsonsmatt · 2025-02-20T23:02:41Z

persistent/Database/Persist/Types/Base.hs

+    , entitySpan :: !(Maybe Span)
+    -- ^ Source code span occupied by this entity. May be absent if it is not
+    -- known.


I guess we wouldn't have this if we skip the quasiquoter entirely and write the instances ourselves. Valid

persistent/persistent.cabal

parsonsmatt · 2025-02-20T23:06:16Z

persistent/test/Database/Persist/THSpec.hs

+personDefBeforeLoc :: SourceLoc
+personDefBeforeLoc = $(TH.lift @_ @SourceLoc . sourceLocFromTHLoc =<< TH.location)


I think there may be a better way of doing this -

do firstLoc <- [d| firstLocation = $(TH.location) |] decls <- share [mkPersistWith sqlSettings ...] [persistUpperCase| ... |] secondLoc <- [d| secondLocation = $(TH.location) |] pure (concat [firstLoc, decls, secondLoc])

This would require a little less spooky magic

Part of the goofiness here is that there's no instance Lift TH.Loc. But you're right, I could extract this.

Okay I give up. I don't think it is possible to do what you suggest. There's no instance Lift TH.Loc, and I believe that you get one value of TH.location per top level splice which means that in the proposed code, the before/after loc values are the same.

This also means it cannot be extracted. Bleh. TH is really hard.

Ah good point. My apologies

It looks like this is failing on GHC 8.8 - complaining about TH.lift @_ @SourceLoc. My guess is that GHC 8.8 has only one type variable for lift

persistent-test/persistent-test.cabal

parsonsmatt · 2025-02-21T17:16:43Z

Build failure is due to persistent-th's benchmark looks like

This is an initial implementation that we can API-compatibly improve in the future by providing more accurate information. However, to do that, we would have to rewrite the entity definition parser with e.g. megaparsec. This is a reasonable course of action but rewriting it is going to be more work and we can ship position info we could improve later with the existing parser.

It winds up making a lot more sense legibility wise to just move the entire module. Maybe the API could be shifted back later (but it uses a pile of internals that really are probably more bothersome to have in a different file?), but this is at least easiest to review since it is quite literally just copy pasting the entire content of the module.

…ies/persistent into jade/entitydef-location

m1-s · 2026-01-09T17:08:21Z

What is the intended replacement for embedEntityDefs given that it is now an internal function?

parsonsmatt · 2026-01-09T19:40:49Z

Could you make an issue with your specific use case? In the meantime, you can depend on the internal module (it is exposed). The docs for it say you can define entities separately and then use that to make a migration. However, that shouldn't be necessary with the way migrateModels works now. I left a comment in the linked issue.

update nix deps

710dab2

lf- force-pushed the jade/entitydef-location branch from e917484 to 0aa83dd Compare February 12, 2025 00:49

parsonsmatt reviewed Feb 20, 2025

View reviewed changes

lf- force-pushed the jade/entitydef-location branch from 0aa83dd to 87864c7 Compare February 26, 2025 19:36

lf- added 4 commits February 26, 2025 11:50

Version 2.15.0.0

7ac74c3

changelog

6b14c82

lf- force-pushed the jade/entitydef-location branch from 87864c7 to 0f6dfdf Compare February 26, 2025 19:54

parsonsmatt added 4 commits March 17, 2025 11:21

fix compilation error

87c568e

Merge branch 'master' into jade/entitydef-location

6e05e36

fix compilation

508abfb

Merge branch 'jade/entitydef-location' of github.com:MercuryTechnolog…

7f9c3b5

…ies/persistent into jade/entitydef-location

parsonsmatt merged commit c6aba87 into yesodweb:master Mar 17, 2025
7 checks passed

parsonsmatt mentioned this pull request Mar 26, 2025

Expose utilities in Database.Persist.TH #1564

Closed

marinelli mentioned this pull request Mar 29, 2025

Make it compatible with the latest text and persistent releases iand675/hs-opentelemetry#179

Merged

m1-s mentioned this pull request Jan 8, 2026

update for persistent 2.17 yesodweb/serversession#42

Open

		personDefBeforeLoc :: SourceLoc
		personDefBeforeLoc = $(TH.lift @_ @SourceLoc . sourceLocFromTHLoc =<< TH.location)

Add position info to EntityDef #1569

Add position info to EntityDef #1569

Uh oh!

Conversation

lf- commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lf- commented Feb 12, 2025

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!

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

parsonsmatt commented Feb 21, 2025

Uh oh!

Uh oh!

m1-s commented Jan 9, 2026

Uh oh!

parsonsmatt commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lf- commented Feb 12, 2025 •

edited

Loading