update Internal.Arena with modern record extensions instead of RecordWildCards #811#818
Conversation
|
@jorisdral ps |
There was a problem hiding this comment.
Thank you for your contribution! We can merge this PR once the single suggestion is resolved.
I think the use of DuplicateRecordFields/NoFieldSelectors/OverloadedRecordDot(and the removal ofRecordWildCards`) like this is what we are looking for. 😃
Note: in the case of the Arena datatype, the record fields already have short names. Once we start using these language extensions in other modules as well, then we should probably rename a bunch of the unnecessarily long field names.
If you're up for doing a follow-up PR, my proposal would be to do a next round of using the record extensions in a number of other modules that also require field renaming. It might be easiest to review the follow-up PR if you don't change all modules all at once, but feel free to use your own judgement of how many updated modules to include in the follow-up PR
…WildCards
Add DuplicateRecordFields, NoFieldSelectors, OverloadedRecordDot
- Replace Arena {..} patterns with dot-access (arena.curr/free/full)
- Replace pure Arena {..} with explicit field assignment, no code logic change just refactored with modern record, part of IntersectMBO#811
…sted and fixed formattinglint
e51c579 to
45d13ea
Compare
|
Thanks @jorisdral to hear the approach with DuplicateRecordFields, NoFieldSelectors, and OverloadedRecordDot aligns with the direction you want. I've resolved that final suggestion, so this should be good to go. Regarding the follow-up PR: I’m definitely up for it. I agree that keeping the module updates bite-sized will make the reviews easier to digest. I'll start looking for modules where we can finally drop those long, prefixed field names now that these extensions are in play. I’m happy to pick a few to start with, but feel free to point me toward any high-priority modules you have in mind! |
jorisdral
left a comment
There was a problem hiding this comment.
Minor: there is "merge main into feature branch" commit now. I personally prefer rebasing over merge commits to keep the git history cleaner. But I'm fine with merging a branch with merge commits too. 😄
I’m happy to pick a few to start with, but feel free to point me toward any high-priority modules you have in mind!
My first thought is maybe the Run module or the Index directory, but feel free to pick anything you like!
Co-authored-by: Joris Dral <jorisjdral@gmail.com>
|
Sure, I will start with run modules. Yeah rebasing is better I will use it, I thought rebasing was for personal projects. I'm happy to adapt to the project's workflow here! |
Pilot for #811
Replaced
RecordWildCardswith the three modern record extensions recommendedin the Haskell Unfolder episode #45.
This is a pilot pr applying the modern record extensions to one internal module
(
Database.LSMTree.Internal.Arena) to get feedback on the approach beforeexpanding to the full codebase.
Changes
Database.LSMTree.Internal.Arena{-# LANGUAGE RecordWildCards #-}DuplicateRecordFields,NoFieldSelectors,OverloadedRecordDotArena {..}wildcard deconstruction with explicit dot-access(
arena.curr,arena.free,arena.full) in:scrambleArenaresetArenaallocateFromArena'pure Arena {..}construction with explicit field assignmentNo logic change — just refactored with modern record extensions.
Testing
cabal build lsm-tree:lib:core: builds successfullycabal test lsm-tree-test --pattern "Arena"→ 2/2 tests pass ✅