Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace emitting MarkedString[] with MarkupContent #179

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Conversation

Strum355
Copy link
Member

closes #162

Required some work in SnapshotLsifCommand.scala due to the new format. Also fixed a load of lint errors that Intellij kept throwing at me 馃檭

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor comments, otherwise looks good.

contents <- hoverResult.getContentsList.asScala
} yield contents.getValue.trim
sym -> hoverMessage.mkString("\n\n")
val hover =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: for comprehensions become hard to read when you add a complicated chain of operations on top of it. I think this diff would read cleaner by keeping the original for comprehension and adding a separate statement instead

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty to push a commit updating the for-comphrension. LGTM 馃憤

@Strum355 Strum355 merged commit a86c1cc into main Apr 26, 2021
@Strum355 Strum355 deleted the nsc/markupcontent branch April 26, 2021 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit MarkupContent instead of MarkedString[] for hover messages
2 participants