search

jdbaldry / jsonnet-language-server

A Language Server Protocol (LSP) server for Jsonnet (https://jsonnet.org).
GNU Affero General Public License v3.0
17 stars 2 forks source link

feat: Front the server with a CLI #30

Closed julienduchesne closed 2 years ago

julienduchesne commented 2 years ago

Opening this as a point of discussion For the vscode extension, I'd like to download/update to the latest released version (with a user prompt). For this, I need to know the currently installed version. Having a CLI --version allows me to query that It also provides a central configuration entrypoint for current and future options

If you think this is a good idea, I can then set up CI to create releases on tags and publish executables (with goreleaser) and the release process can be configured to pass the correct ldflags

Signed-off-by: Julien Duchesne julien.duchesne@grafana.com

jdbaldry commented 2 years ago

I think this is a great point for discussion.

I fully support adding a version flag that can be used for easier install and upgrade UX. I don't want to be too prescriptive about how we achieve this but I was originally thinking that the additional dependency isn't justified and we could have our own simplified command line parsing. That being said, I'm not a big fan of the built-in flag package and so would probably use pflag adding in a dependency anyway. I know of a few CLI packages for Go and don't have any strong preference. Reading through the urfave documentation, it looks to provide a lot of utility. As I was writing this and thinking about the nature of the jsonnet-language-server application, it seems like we may benefit from using a CLI framework more than I originally thought since we are going to likely need to support a number of configuration options like formatter and interpreter flags so I'm even more sympathetic with adding the dependency.

I'm less keen on the move into pkg. I'm not sure there is any API within this package that I would consider supporting from a library perspective and I wouldn't want to imply that as this change does. I'd also be keen to have a more descriptive package name though I can't think what that would really be right now. Perhaps server? Do we need the pkg changes to facilitate the versioning or CLI changes this PR introduces or could it be deferred?

julienduchesne commented 2 years ago

I think this is a great point for discussion.

I fully support adding a version flag that can be used for easier install and upgrade UX. I don't want to be too prescriptive about how we achieve this but I was originally thinking that the additional dependency isn't justified and we could have our own simplified command line parsing. That being said, I'm not a big fan of the built-in flag package and so would probably use pflag adding in a dependency anyway. I know of a few CLI packages for Go and don't have any strong preference. Reading through the urfave documentation, it looks to provide a lot of utility. As I was writing this and thinking about the nature of the jsonnet-language-server application, it seems like we may benefit from using a CLI framework more than I originally thought since we are going to likely need to support a number of configuration options like formatter and interpreter flags so I'm even more sympathetic with adding the dependency.

I'm less keen on the move into pkg. I'm not sure there is any API within this package that I would consider supporting from a library perspective and I wouldn't want to imply that as this change does. I'd also be keen to have a more descriptive package name though I can't think what that would really be right now. Perhaps server? Do we need the pkg changes to facilitate the versioning or CLI changes this PR introduces or could it be deferred?

You're absolutely right about the pkg thing. It's a bad reflex of mine. I put commands in cmd and library stuff in pkg invariably. Mostly because I try to follow this: https://github.com/golang-standards/project-layout, and I really don't like putting code in internal (I think it's an ugly name)

In any case, I moved everything back to the main dir. I really didn't need to do any moves for this PR 😅

jdbaldry commented 2 years ago

Thanks, new PR looks great, will endeavor to properly review soon.

jdbaldry commented 2 years ago

How do you feel about https://github.com/jdbaldry/jsonnet-language-server/pull/35?

julienduchesne commented 2 years ago

How do you feel about #35?

See now, I based this PR on #35 but I used flag instead. Same behavior as your PR but with a bit less boilerplate and easier to maintain

jdbaldry commented 2 years ago

Thanks for taking a look at #35 :)

I agree that with your sentiment that manual arg parsing is definitely open to edge cases but I'm still on the fence as to whether the additional dependency is worth it. I'm not a fan of the flag package since it doesn't provide POSIX short options and I really would like to prioritize parity with the jsonnet command line options where we can.

For now, I've created a new release with the version option included so that you can be unblocked on any automated installation in VSCode.

julienduchesne commented 2 years ago

👍 Great, I was just proposing things. Thanks for releasing the version!