Code action for OPA fmt #630

charlieegan3 · 2024-04-10T10:33:35Z

Screen.Recording.2024-04-10.at.11.31.26.mov

Signed-off-by: Anders Eknert <anders@styra.com>

Signed-off-by: Charlie Egan <charlie@styra.com>

srenatus · 2024-04-10T11:21:15Z

internal/lsp/command.go

+func FmtCommand(args []string) Command {
+	return Command{
+		Title:     "Format using opa-fmt",
+		Command:   "regal.fmt",


Would you happen to know if there's a well-known command name or some other mechanism at play when an IDE/editor does "format on save" via LSP? What I'm wondering is what we'd need to do to have that just work. If it's command-name-based, we'd need to adjust this. But I know way too little about LSP myself 😅

https://zed.dev/docs/configuring-zed#lsp this makes me think there's no convention...

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_formatting

And yeah, that'll be trivial to add with this code in place as the response is the same.

srenatus

I'm not the most competent reviewer, but I've reviewed it anyways 🙃 (except for the copied-over diff parts)

srenatus · 2024-04-10T11:22:39Z

internal/lsp/server.go

@@ -121,6 +134,8 @@ func (l *LanguageServer) Handle(
 		return struct{}{}, nil
 	}

+	fmt.Fprintf(l.errorLog, "%s\n", string(util.MustMarshalJSON(req)))


Suggested change

fmt.Fprintf(l.errorLog, "%s\n", string(util.MustMarshalJSON(req)))

fmt.Fprintln(l.errorLog, string(util.MustMarshalJSON(req)))

(if it was desired to keep this line)

Thanks, I don't think we need this tbh. Will remove, I have some ideas to improve the logging in the server, but will do in another PR.

srenatus · 2024-04-10T11:23:12Z

internal/lsp/server.go

+	for _, diag := range params.Context.Diagnostics {
+		if diag.Code == "opa-fmt" {
+			actions = append(actions, CodeAction{
+				Title:       "Format using opa fmt",


This is cool ✨

anderseknert

Awesome! Left a few comments, but nothing big.

internal/lsp/messages.go

internal/lsp/server.go

internal/lsp/format.go

This is not really the best place to do this, so I'll do it properly in another PR. Signed-off-by: Charlie Egan <charlie@styra.com>

This is now used where it's used in the spec. Signed-off-by: Charlie Egan <charlie@styra.com>

Signed-off-by: Charlie Egan <charlie@styra.com>

While working on #630, I needed to be able to see requests, responses and notifications to the server from the client and from the server to the client. I learned that there is functionality in the jsonrpc2 lib that we use to do this nicely. This PR implements a new logging configuration to make use of this using an adapted version of the LogMessage function that is found in the library and using the OnRecv and OnSend functions. Because of this, I have removed the logging responsibility from the server and moved it to the connection. The server now should log errors only and that should be the default mode of operation. Enabling verbose logging will show you all messages being sent on the connection. ExcludeMethods and IncludeMethods can be used to filter the stream of messages when you are interested in monitoring or ignoring certain flows of messages. I know that this will be useful for me and I hope that this is a sensible enough way to do this overall. Signed-off-by: Charlie Egan <charlie@styra.com>

* Refactor LSP logging While working on #630, I needed to be able to see requests, responses and notifications to the server from the client and from the server to the client. I learned that there is functionality in the jsonrpc2 lib that we use to do this nicely. This PR implements a new logging configuration to make use of this using an adapted version of the LogMessage function that is found in the library and using the OnRecv and OnSend functions. Because of this, I have removed the logging responsibility from the server and moved it to the connection. The server now should log errors only and that should be the default mode of operation. Enabling verbose logging will show you all messages being sent on the connection. ExcludeMethods and IncludeMethods can be used to filter the stream of messages when you are interested in monitoring or ignoring certain flows of messages. I know that this will be useful for me and I hope that this is a sensible enough way to do this overall. Signed-off-by: Charlie Egan <charlie@styra.com> * Inline logging config Signed-off-by: Charlie Egan <charlie@styra.com> * Remove VerboseLogging server option for good Also use an io.Writer for errors Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Charlie Egan <charlie@styra.com>

* WIP code action Signed-off-by: Anders Eknert <anders@styra.com> * Support sending of workspace/applyEdits Signed-off-by: Charlie Egan <charlie@styra.com> * Fix linter errors Signed-off-by: Charlie Egan <charlie@styra.com> * Remove logging of unknown messages This is not really the best place to do this, so I'll do it properly in another PR. Signed-off-by: Charlie Egan <charlie@styra.com> * Add OptionalVersionedTextDocumentIdentifier This is now used where it's used in the spec. Signed-off-by: Charlie Egan <charlie@styra.com> * Add license for ComputeEdits Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Anders Eknert <anders@styra.com> Signed-off-by: Charlie Egan <charlie@styra.com> Co-authored-by: Anders Eknert <anders@styra.com>

* Refactor LSP logging While working on StyraInc#630, I needed to be able to see requests, responses and notifications to the server from the client and from the server to the client. I learned that there is functionality in the jsonrpc2 lib that we use to do this nicely. This PR implements a new logging configuration to make use of this using an adapted version of the LogMessage function that is found in the library and using the OnRecv and OnSend functions. Because of this, I have removed the logging responsibility from the server and moved it to the connection. The server now should log errors only and that should be the default mode of operation. Enabling verbose logging will show you all messages being sent on the connection. ExcludeMethods and IncludeMethods can be used to filter the stream of messages when you are interested in monitoring or ignoring certain flows of messages. I know that this will be useful for me and I hope that this is a sensible enough way to do this overall. Signed-off-by: Charlie Egan <charlie@styra.com> * Inline logging config Signed-off-by: Charlie Egan <charlie@styra.com> * Remove VerboseLogging server option for good Also use an io.Writer for errors Signed-off-by: Charlie Egan <charlie@styra.com> --------- Signed-off-by: Charlie Egan <charlie@styra.com>

anderseknert and others added 3 commits April 10, 2024 11:33

WIP code action

e945856

Signed-off-by: Anders Eknert <anders@styra.com>

Support sending of workspace/applyEdits

25cce46

Signed-off-by: Charlie Egan <charlie@styra.com>

Fix linter errors

a3a6a42

Signed-off-by: Charlie Egan <charlie@styra.com>

charlieegan3 force-pushed the code-action-fmt branch from e6ecb4c to a3a6a42 Compare April 10, 2024 10:33

srenatus reviewed Apr 10, 2024

View reviewed changes

srenatus approved these changes Apr 10, 2024

View reviewed changes

anderseknert reviewed Apr 10, 2024

View reviewed changes

internal/lsp/messages.go Show resolved Hide resolved

internal/lsp/server.go Show resolved Hide resolved

internal/lsp/format.go Show resolved Hide resolved

charlieegan3 added 3 commits April 10, 2024 13:10

Remove logging of unknown messages

13ed42b

This is not really the best place to do this, so I'll do it properly in another PR. Signed-off-by: Charlie Egan <charlie@styra.com>

Add OptionalVersionedTextDocumentIdentifier

7d0946d

This is now used where it's used in the spec. Signed-off-by: Charlie Egan <charlie@styra.com>

Add license for ComputeEdits

c1a0796

Signed-off-by: Charlie Egan <charlie@styra.com>

anderseknert approved these changes Apr 10, 2024

View reviewed changes

charlieegan3 merged commit de1ff4b into main Apr 10, 2024
3 checks passed

charlieegan3 deleted the code-action-fmt branch April 10, 2024 12:39

charlieegan3 mentioned this pull request Apr 10, 2024

Implement LSP code action for opa-fmt violation #616

Closed

charlieegan3 mentioned this pull request Apr 10, 2024

Refactor LSP logging #632

Merged

anderseknert mentioned this pull request Apr 10, 2024

Implement textDocument.formatting #638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code action for OPA fmt #630

Code action for OPA fmt #630

charlieegan3 commented Apr 10, 2024

srenatus Apr 10, 2024

srenatus Apr 10, 2024

anderseknert Apr 10, 2024

srenatus left a comment

srenatus Apr 10, 2024

charlieegan3 Apr 10, 2024

srenatus Apr 10, 2024

anderseknert left a comment

	fmt.Fprintf(l.errorLog, "%s\n", string(util.MustMarshalJSON(req)))
	fmt.Fprintln(l.errorLog, string(util.MustMarshalJSON(req)))

Code action for OPA fmt #630

Code action for OPA fmt #630

Conversation

charlieegan3 commented Apr 10, 2024

srenatus Apr 10, 2024

Choose a reason for hiding this comment

srenatus Apr 10, 2024

Choose a reason for hiding this comment

anderseknert Apr 10, 2024

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

srenatus Apr 10, 2024

Choose a reason for hiding this comment

charlieegan3 Apr 10, 2024

Choose a reason for hiding this comment

srenatus Apr 10, 2024

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment