discussion: naming fields in ConstraintSolver's exception types #6604
Conversation
| exception PossibleOverload of DisplayEnv * string * exn * range | ||
| exception UnresolvedOverloading of DisplayEnv * exn list * string * range | ||
| exception UnresolvedConversionOperator of DisplayEnv * TType * TType * range | ||
| exception ConstraintSolverTupleDiffLengths of displayEnv: DisplayEnv * TType list * TType list * range * range |
There was a problem hiding this comment.
Nit: We don't normally have these sorts of whitespace additions. @dsyme what are your thoughts here?
There was a problem hiding this comment.
I noticed that but let it slide, it's ok, it was inconsistent in the signature file in any case.
There was a problem hiding this comment.
Do you want me to clear the spacing in #6596, the overload related exceptions are changing.
There was a problem hiding this comment.
Could we clarify if, for example, such alignement is proscribed/discouraged/recommended/prescribed for specific or any situations in the guidelines:
Do the maintainers want to set a bar somewhere else than "code should have reasonable format, good effort on readability, and fit in the context"?
How does that get addressed in the guidelines & our CI/review workflow in the future?
I agree that code alignment makes editing slightly more involved in few cases, this is the main drawback I see, but the readability pros generally outweights it (subjective, for me).
I also agree the maintainers have the command on that when it comes to review/integrate contributions.
To look further around us, have you noticed jetbrains has been adding support for advanced settings for a bit: https://blog.jetbrains.com/dotnet/2017/11/28/aligning-code-columns-resharper-rider/ ?
It indicates that the demand for such features exist.
NB: This is not a rant 😄, let me know how to fix the code in the other PR.
There was a problem hiding this comment.
I personally am not a fan of column alignment (and an F#-specific variant, arrow aligment in match expressions, is not recommended as per the official style guide)
I don't think we have a hard and fast rule, but uniformity is what's preferred here above all else. That's why lots of whitespace normalization PRs were recently brought in.
There was a problem hiding this comment.
Yes we've been removing column alignment. There are some places we still use it though, e.g. tast.fs ValFlags, where it is extremely useful
Mostly we've been removing it to add space for doc comments, e.g.
let foo1 xxx yyy zzz = align
let foo2 x y z = align
becomes
/// comment
let foo1 xxx yyy zzz = align
/// comment
let foo2 x y z = align
Not much point in keeping the alignment when comments are added
4 participants
I've seen those
Data0@, etc. in debugging around compiler error in ConstraintSolver.fs and I believe the code is moving toward naming those fields.Putting up a PR to gather feedback on the issue from people working on tooling / compiler.