Consolidate and share logic for Records and AnonymousRecords #2750
Conversation
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
Hi Josh, I see where you are going with this, but I find it to be somewhat questionable.
I think extracting helper functions like genCopyExpr is more interesting than the active pattern.
As mentioned, the types differ enough that combining them no longer makes that obvious.
One alternative I do see is to split the types into different levels.
Having Expr like Record, InheritRecord and StructAnonRecord.
Where the Record case shares everything that SynExpr.Record and SynExpr.AnonRecord actually share. And InheritRecord and StructAnonRecord highlight where they actually differ.
src/Fantomas.Core/CodePrinter.fs
Outdated
| | Expr.IfThenElse _ -> true | ||
| | _ -> false | ||
|
|
||
| let (|RecordOrAnonRecord|_|) (expr: Expr) = |
There was a problem hiding this comment.
I'm not really a fan of this active pattern. It somewhat ruins the magic that you access an exact node type from SyntaxOak in CodePrinter.
The anonymous records used are also not consistent with anything else in this codebase.
If you were going to make this split, it should be in SyntaxOak where we have a shared Node for the two when possible.
There was a problem hiding this comment.
That makes a lot of sense. My original plan was essentially doing what my active pattern did but with a single shared Node, but I had issues because of something about trivia which led me to try the active pattern instead. But consolidating via Node inheritance makes a ton of sense. My goal was mainly to reduce duplication and make it easier for one change to fix a problem in both, but I also don't want to sacrifice readability or make anything harder to follow. Also didn't love having to have additional cases that failed because the exhaustive check failed. I'll try and refactor to your approach here instead.
src/Fantomas.Core/CodePrinter.fs
Outdated
| Fields = record.Fields | ||
| ClosingBrace = record.ClosingBrace | ||
| Range = record.Range | ||
| AnonRecord = None |
There was a problem hiding this comment.
This exact field is a code smell on why these types cannot be combined.
There are subtle differences but in essence, one cannot be completely the other.
The code that needs to deal with this type will inevitably need to place if/else.
The anonymous record cannot have inherit and the regular record cannot have struct. That isn't very obvious from this refactor.
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
A few more nits, but I really like this solution.
Great job!
|
|
||
| type ExprAnonRecordNode | ||
| ( | ||
| isStruct: bool, |
There was a problem hiding this comment.
You don't really need the boolean if you were to rename this to ExprAnonRecordStructNode.
A non-struct anonymous record would fit perfectly in ExprCopyableRecordNode, so you only really need a new type for when it has the struct in it.
There was a problem hiding this comment.
There are still a few differences in non-struct records that I've noticed (see genMultilineAnonCrampedFields). If those can be consolidated, that would be great, but I wasn't able to get everything to work without those changes
nojaf
force-pushed
the
v6.0
branch
2 times, most recently
from
a5ea3b6 to
fde30d4
Compare
Introduce active pattern to join implementation for Record & AnonRecords Consolidate some more logic to fix an edge case Consolidate a bit more Remove unintentional change, cleanup Refactor to remove active pattern
josh-degraw
force-pushed
the
record-consolidate
branch
from
c6be136 to
032e994
Compare
nojaf
left a comment
nojaf
left a comment
There was a problem hiding this comment.
Thanks, this looks good! I've added a couple of XML documents as it can get challenging to find out what each helper function does at the point of invocation.
I do believe we can address the cramped logic even further. I might try it out in a separate PR. Thanks again for all your help!
* Remove AnonRecordFieldNode * Consolidate a lot of logic Introduce active pattern to join implementation for Record & AnonRecords Consolidate some more logic to fix an edge case Consolidate a bit more Remove unintentional change, cleanup Refactor to remove active pattern * Add ExprCopyableRecordNode subclass to better separate concerns * Clean up a bit * Remove a type, rename some others * Inline some one-off functions * Format CodePrinter.fs * Add some additional XML documentation. --------- Co-authored-by: nojaf <[email protected]>
* Remove AnonRecordFieldNode * Consolidate a lot of logic Introduce active pattern to join implementation for Record & AnonRecords Consolidate some more logic to fix an edge case Consolidate a bit more Remove unintentional change, cleanup Refactor to remove active pattern * Add ExprCopyableRecordNode subclass to better separate concerns * Clean up a bit * Remove a type, rename some others * Inline some one-off functions * Format CodePrinter.fs * Add some additional XML documentation. --------- Co-authored-by: nojaf <[email protected]>
* Remove AnonRecordFieldNode * Consolidate a lot of logic Introduce active pattern to join implementation for Record & AnonRecords Consolidate some more logic to fix an edge case Consolidate a bit more Remove unintentional change, cleanup Refactor to remove active pattern * Add ExprCopyableRecordNode subclass to better separate concerns * Clean up a bit * Remove a type, rename some others * Inline some one-off functions * Format CodePrinter.fs * Add some additional XML documentation. --------- Co-authored-by: nojaf <[email protected]>
2 participants
It's happened several times that something that gets fixed with records that didn't get fixed for anonymous records. Since they share so much in the style guide, I wanted to see if I could refactor
genExprto handle them both at the same time so they could share as much logic as possible. I refactored both nodes to use the same field node type, and added an active pattern to join them together and then only handle differences explicitly as needed. It seems to work perfectly to me and I actually found an existing bug that was fixed for records but not anonymous records in the process 🙃.