search

bitemyapp / esqueleto

New home of Esqueleto, please file issues so we can get things caught up!
BSD 3-Clause "New" or "Revised" License
370 stars 107 forks source link

Importing Database.Esqueleto confusing in 4.0.0.0 #368

Open m4dc4p opened 11 months ago

m4dc4p commented 11 months ago

4.0.0.0 (esqueleto-next/ https://github.com/bitemyapp/esqueleto/pull/325) updates Database.Esqueleto to the new syntax. I was migrating some code and struggled with confusing error messages ("Could not deduce (Database.Esqueleto.Internal.Internal.SqlSelect a0 (Entity record)) arising from a use of ‘select’") for some time before realizing I just needed to change Database.Esqueleto to Database.Esqueleto.Legacy.

I know the deprecation warning has been on Experimental forever, and possibly on Database.Esqueleto, but even so this caused me a good hour of frustration.

Is there some way to highlight the change? A migration guide or deprecation warning?

Thanks for all your excellent work!

parsonsmatt commented 11 months ago

There is a warning on Database.Esqueleto since version 3.5.0.0 which was uploadedin May of 2021, and has been on Stackage LTS since 18.1, released July 5th 2021.

I'm curious what your workflow is that you missed this warning.

m4dc4p commented 11 months ago

I work on a large application with hundreds of modules and a lot of dependencies. We use nix for building in CI and our dev environment. The warning became noise in all that log spam. Additionally, the change needed to happen in dependent libraries, so I was trying to patch a dependency rather than our own code, so it just wasn't immediately obvious to me that I needed to update the import.

A warning at time-of-breakage (that is, on this release) would have saved me an hour or so.

parsonsmatt commented 11 months ago

esqueleto-4 has not been released yet - you're on a pre-release git branch, so that does give us some time to improve this experience.

Still, I'm not exactly sure what you're proposing or expecting. Do you want for esqueleto-4 to have a Database.Esqueleto that with a warning like This module exports the new syntax. If you still need legacy syntax, import Database.Esqueleto.Legacy, but beware that it will be deleted in an upcoming major version. ?

I'd like for esqueleto-4 to be the "import Database.Esqueleto to use new syntax without any warnings." Do you suppose that an esqueleto-3.6 which does the switch + warn would have helped? I don't think so, since you jumped to esqueleto-4 instead of "most recent Hackage release," but I'm happy to hear your thoughts.

m4dc4p commented 11 months ago

Do you want for esqueleto-4 to have a Database.Esqueleto that with a warning like [...]

That pretty much would have avoided the problem for me.

Do you suppose that an esqueleto-3.6 which does the switch + warn would have helped?

I picked up this branch as I wanted to try the new window function support, so in that sense I'm an outlier - I definitely chose the pre-release branch. Its pretty hard to say if a 3.6 would help others or not. FWIW, I ran into this problem when recompiling persistent-pagination. Considering how that library is impacted by someone upgrading esqueleto might help clarify what you want to do.

Appreciate the discussion and hope this helps!