-
Notifications
You must be signed in to change notification settings - Fork 1.1k
formatting: Format presentation-compiler with scalafmt #24977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| refineParams(f, level + 1) | ||
| case _ => getRefinedParams(method.symbol.info, level) | ||
| refineParams(method, 0) | ||
| end baseParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caught my eye as strictly worse. Maybe the line comments are visually as good as blank lines? Maybe the tailrec annotations are noise. Maybe the weird code structure, 3 long defs and one result expr.
I hope we can "squirrel away" local helper methods:
val baseParams =
return refineParams(method, 0)
def refineParams(...) = ???
def etc = ???
That is not for today, however.
| /** A heuristic for checking if the passed arguments match the method's arguments' types. For non-polymorphic methods | ||
| * we use the subtype relation (`<:<`) and for polymorphic methods we use a heuristic. We check the args types not | ||
| * the result type. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if doc comments are formatted, they should be formatted to the one true style, which is not this one.
| def doMatch( | ||
| allArgsProvided: Boolean, | ||
| span: Span | ||
| span: Span, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like trailing comma but for things subject to update, such as lists of things (data). Signatures change infrequently so don't see churn. I don't happen to prefer vertical params.
| config | ||
| .symbolPrefixes() | ||
| .nn | ||
| .asScala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting because it looks wrong. x.nn.asScala. I wouldn't want asInstanceOf on a separate line, but why?
| tail.foldLeft(direct(head.nameBackticked))((acc, next) => | ||
| Select(acc, next.nameBackticked) | ||
| ) | ||
| tail.foldLeft(direct(head.nameBackticked))((acc, next) => Select(acc, next.nameBackticked)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to justify this compression. even if it works later with colon syntax.
| * @param indexedContext | ||
| * A context of the position where the autoImport is invoked | ||
| * @param config | ||
| * A presentation compiler config, this is used for renames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style makes me hate Scaladoc. Why?
@param do
Doe, a deer, a female deer
@param ray
A drop of golden sun
| path.isScalaCLIGeneratedFile || | ||
| path.isWorksheet | ||
| path.isScalaCLIGeneratedFile || | ||
| path.isWorksheet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controversial! Also, it's not converting to leading infix, which is the other crucial innovation for readability.
avant-garde:
if a
|| b
|| c
then x
else y
| val indexedContext = IndexedContext(pos)( | ||
| using Interactive.contextOfPath(path)(using newctx) | ||
| val indexedContext = IndexedContext(pos)(using | ||
| Interactive.contextOfPath(path)(using newctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the rule here, but it could leverage "leading paren".
| case Nil => None | ||
| case _ :: (app @ Apply(fun, args)) :: _ if args.exists(ImplicitParameters.isSyntheticArg(_)) => Some(app) | ||
| case found :: _ => Some(found) | ||
| case found :: _ => Some(found) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ameliorated by
case Nil => 1
case long_______pattern
if long_________guard => 2
case _ => 3
| * We could in theory save this fresh driver for reuse, but it is a choice between extra memory usage and speed. | ||
| * The scenario in which "CURSOR" is applied (empty query or query equal to any keyword) has a slim chance of | ||
| * happening. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous asterisk style is better because it has a nice gutter.
| * Iterator[Int]#GroupedIterator[Int]) | ||
| * | ||
| * With completion being run at line 4 at @@: 4| $1$.sliding@@[Int](size, step) | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obvious that formatter requires quality use of markdown. I would need some retraining.
| * it's required to modify actual code by additional Ident. | ||
| * | ||
| * Otherwise, completion poisition doesn't point at any tree because scala parser trim end position to the last | ||
| * statement pos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example where a shorter line length for comments make them easier to read in an editor, that is, unrendered.
|
I'm not an expert, but I added knee-jerk observations as feedback (rather than objections per se). Very interesting to see the results! The only syntax I feel strongly about is updating |
We still don't have rule that would remove braces if a certain number of blanks is available, we can implement it if the new style gets accepted wider. For now in this PR I add options to:
I also run the rule to remove optional braces, but disabled it since it removed them in longer classes as well, we would need to add a similar rule to what we have with end markers.