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

new-scala-file command #12

Merged
merged 2 commits into from
May 15, 2020
Merged

new-scala-file command #12

merged 2 commits into from
May 15, 2020

Conversation

landerlo
Copy link
Contributor

WIP new-scala-file. Still needs some polishing but this is a working new-scala-file command.

@landerlo landerlo force-pushed the new-scala-file branch 2 times, most recently from 2938fb2 to bbb520a Compare May 13, 2020 21:40
@ckipp01
Copy link
Member

ckipp01 commented May 14, 2020

Thanks a lot for starting to work on this! How does this actually work though? Looking at it, I'd expecting that after the executeCommand is called (metals.new-scala-file) then Metals will respond with a window/showMessageRequest with the possible options. However, right now that's unhandled.

@landerlo landerlo force-pushed the new-scala-file branch 2 times, most recently from d500579 to b9fa0e5 Compare May 14, 2020 12:38
@landerlo
Copy link
Contributor Author

landerlo commented May 14, 2020

Sorry, I forgot to update the setup file, I pushed the changes again with the new setup you need to change the setup as in the new setup example I pushed, declaring the inputBox and quickPick capabilities, so metals uses that.

The cancelling also works now.
I removed my 'rant' about metals not using workspaceEdits to create the file (I still think it'd have been better design than creating the file outside). But I see that it executes a GoTo to the created file, so we'll get the focus when we implement the GoTo command.

@landerlo
Copy link
Contributor Author

Ok, the focus change is completed now, so this completes the functionality.
I was missing it uses GoToLocation commands to change focus.
You need to update your setup to the example setup here.

We'll need to think a way to have to manage setup and not having to change user config , when metals-nvim add endpoints or capabilities like in this PR.

Maybe instead of having to using the lua << whole setup code, have something like setup_nvim_metals { myOverrides }

Provides metals/quickPick and metals/inputBox capabilities
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Great job on this! Just a few comments/questions.

lua/metals.lua Show resolved Hide resolved
lua/metals.lua Outdated Show resolved Hide resolved
lua/metals.lua Outdated
end
end

M.new_scala_file = function(directory_opt)
Copy link
Member

Choose a reason for hiding this comment

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

In what scenario is the directory_opt used here? Is this mainly for someone to use if they want to pass in a parameter?

Copy link
Contributor Author

@landerlo landerlo May 14, 2020

Choose a reason for hiding this comment

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

I've added the name_opt as well, so if any of those are passed it won't be requested in the UI. Passing the name is useful for the new symbol quickfix when we create the symbol under cursor. VsCode has that quickfix. It can be reused if we implement that code action, it's simply calling this function with the directory=nil, and name= word under cursor or missing symbol.
The directory is probably less useful but it's still an option supported by metals. In most cases people will just cd, but I can see -myself if I hafe time- using a fuzzy selection for all the existing packages and selecting destination. Maybe even using a full name: org.corp.NewClass that fills both the folder and the name into one call. Anyway, it doesn't add much code to support it. But feel free to remove if you think it's goldplatting.

lua/metals/util.lua Show resolved Hide resolved
@ckipp01
Copy link
Member

ckipp01 commented May 14, 2020

I removed my 'rant' about metals not using workspaceEdits to create the file (I still think it'd have been better design than creating the file outside).

I don't exactly remember the reason, but I remember we had this conversation on the Metals side of things I believe and obviously didn't go that route. I don't remember the exact reasoning.

We'll need to think a way to have to manage setup and not having to change user config , when metals-nvim add endpoints or capabilities like in this PR.

Maybe instead of having to using the lua << whole setup code, have something like setup_nvim_metals { myOverrides }

Yea I've been thinking about this a bit lately and trying to figure out what a good balance between just offering everything and having the user set it up, or just doing it for them. I've noticed some plugins like completion-nvim for example just straight up override callbacks etc and all they require is to be included on_attach(). But I guess I'm not 100% sure that all users will want all the features this will hopefully have or not.

I also am watching this and seeing what happens. There is the chance that if the entire install goes away, it might make sense to just use Coursier here and add in the install/update/uninstall functionality. If we do that, then the next question is do we just handle the entire setup and not require nvim-lsp at all. /shrug

@landerlo
Copy link
Contributor Author

If there is no update we can have a single nvim_setup that does everything, and user call it, and can override bindings or Commands after calling it. Maybe that f with all batteries included can be calling three funs for more fine grained like setup() (does the attach), default_bindings(), and default_commands() so somebody can pick and choose to not have any unwanted bindings. I personally like to have new functionality and bindings added with as little config as possible, but offering the pick and choose possibility. For me the test is that a feature like this shouldn't require me any changes if I've gone batteries included. NewScalaFile appears magically when I PlugUpdate
nvim-metals

lua/metals.lua Outdated Show resolved Hide resolved

execute_command({
command = 'metals.new-scala-file';
arguments = args_string_array
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize this before, but on the Metals side, this is a bit odd to me that it's an array of two optional strings rather than

{
  uri?: string,
  filename?: string
}

@ckipp01
Copy link
Member

ckipp01 commented May 15, 2020

If there is no update we can have a single nvim_setup that does everything, and user call it, and can override bindings or Commands after calling it. Maybe that f with all batteries included can be calling three funs for more fine grained like setup() (does the attach), default_bindings(), and default_commands() so somebody can pick and choose to not have any unwanted bindings. I personally like to have new functionality and bindings added with as little config as possible, but offering the pick and choose possibility. For me the test is that a feature like this shouldn't require me any changes if I've gone batteries included. NewScalaFile appears magically when I PlugUpdate
nvim-metals

I fully agree with this. I think it makes sense to merge this is, and then start moving towards a setup like that.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for work on this. LGTM 🎉

@ckipp01 ckipp01 merged commit 9eb8e7b into scalameta:master May 15, 2020
@ckipp01 ckipp01 mentioned this pull request May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants