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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in server property for doctor-format and implement json format #1235

Merged
merged 3 commits into from Dec 31, 2019

Conversation

@ckipp01
Copy link
Member

ckipp01 commented Dec 31, 2019

fixes #1234

This will allow for a new server property to be set metals.doctor-format that currently has two options, html and json. This will allow for clients like coc-metals to more easily parse the information and display it textually.

Here are a couple examples of the current format:

{
  "name": "Metals Doctor",
  "headerText": "These are the installed build targets for this workspace. One build target corresponds to one classpath. For example, normally one sbt project maps to two build targets: main and test.",
  "messages": [
    {
      "title": "⚠️  No build targets were detected in this workspace so most functionality won't work.",
      "recommendations": [
        "Make sure the workspace directory '/Users/ckipp/Documents/scala-workspace/test-project' matches the root of your build.",
        "Try removing the directories .metals/ and .bloop/, then restart metals And import the build again."
      ]
    }
  ]
}

and

{
  "name": "Metals Doctor",
  "headerText": "These are the installed build targets for this workspace. One build target corresponds to one classpath. For example, normally one sbt project maps to two build targets: main and test.",
  "targets": [
    [
      {
        "Build Target": "root-test",
        "Scala": "2.12.10",
        "Diagnostics": "",
        "Goto definition": "",
        "Completions": "",
        "Find references": "",
        "Recommendation": ""
      },
      {
        "Build Target": "root",
        "Scala": "2.12.10",
        "Diagnostics": "",
        "Goto definition": "",
        "Completions": "",
        "Find references": "",
        "Recommendation": ""
      }
    ]
  ]
}
Copy link
Member

olafurpg left a comment

Thank you for fixing this! I think it's a great idea to support alternative formats (it's totally reasonable that some editors don't have a full-blown HTML renderer!)

@ckipp01 ckipp01 force-pushed the ckipp01:json-doctor branch from 0598245 to 0f46a7d Dec 31, 2019
@ckipp01 ckipp01 requested a review from olafurpg Dec 31, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Dec 31, 2019

Thanks for the quick review @olafurpg!

Copy link
Member

gabro left a comment

Nice! 👏
I have some comments on the approach: I would decouple the data from the format encoding a bit more. See the inline comments

@ckipp01 ckipp01 force-pushed the ckipp01:json-doctor branch from deacfe2 to 6605274 Dec 31, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Dec 31, 2019

Alright, I took a stab at what you were mentioning @gabro. I'll be honest, I no idea if that was the approach you were thinking or if I went about that correctly. I'd very much like some feedback on how to improve it.

@ckipp01 ckipp01 force-pushed the ckipp01:json-doctor branch from 6605274 to 6462d52 Dec 31, 2019
@ckipp01 ckipp01 requested a review from gabro Dec 31, 2019
@gabro
gabro approved these changes Dec 31, 2019
Copy link
Member

gabro left a comment

@ckipp01 👍 thanks for refactoring this, I think it reads better now. I might take a stab at using Writer from upickle, but that's definitely out of scope now.

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Dec 31, 2019

@ckipp01 👍 thanks for refactoring this, I think it reads better now. I might take a stab at using Writer from upickle, but that's definitely out of scope now.

oooo, I didn't even know about this as I was only looking at the uJson package. Thanks for pointing that out!

@ckipp01 ckipp01 force-pushed the ckipp01:json-doctor branch from 6462d52 to 2c8c782 Dec 31, 2019
@ckipp01 ckipp01 merged commit feec5db into scalameta:master Dec 31, 2019
12 checks passed
12 checks passed
windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@ckipp01 ckipp01 deleted the ckipp01:json-doctor branch Dec 31, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Jan 1, 2020

ujson doesn’t include a serialization type class or macros, it’s just a plain JSON library. The higher-level functionality is available in upickle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.