Code action: convert type to module

[aspeddro]

Add basic support to convert type to module.

• Support types inside module?

Current state:

• Does not support types inside module
• Does not support recursive types.

TODO:

• Update changelog

Demo:

screencast-2022-07-29_20.43.26.mp4

Close #430

[zth]

Awesome! 😄 I'll let @cristianoc look at the OCaml parts more in depth. I noticed there's a few refactors - might be good to add a comment for each of those and detail what they do/why.

[aspeddro]

To return uri scheme.

[cristianoc]

Forgot why this does not just return the string it starts with?

[aspeddro]

Was breaking code actions in neovim client because it was not returning uri scheme from LSP spec.

Example: Add type annotation

[{"title": "Add type annotation", "kind": "refactor.rewrite", "edit": {"documentChanges": [{
  "textDocument": {
  "version": null,
  "uri": "/home/pedro/Desktop/learning-rescript/src/intro/Codeaction.res"
  },
  "edits": [{
  "range": {"start": {"line": 20, "character": 5}, "end": {"line": 20, "character": 5}},
  "newText": ": int"
  }]
  }]}}]

In vscode it works without problem

[cristianoc]

Sorry what I meant to ask is: what does this composition of functions do?
Let's put it into a single function with a name suggesting what it does, and use that function instead. So it's less likely that the same issue will pop up somewhere else by copy paste and change in future.

[cristianoc]

Haven't looked at the ocaml code that -- can do that soon.
I was first wondering about the big picture, perhaps @zth can help a bit there.
Trying to gauge what are going to be the actions of this kind supported in future, and how they play well with each other.
If we're just experiments and are happy to add/remove these kinds of things freely, then we can just go ahead, mark as experimental etc.

The questions that immediately come to mind are: is this too narrow a use case? Or is it general enough?
If we have 8 of these little things, how noisy is the UI going to get when lots of pieces of code trigger several actions etc.

So my question at the moment is: should we worry about these aspects already? Or just experiment first with a few features see how they feel and how they are used, knowing we might revise perhaps even heavily later?

[cristianoc]

Also, should we have an inverse action? To move something out of a module?

[cristianoc]

Some usage questions: this does not seem to trigger an action:

type rec tree = Node({name: string, leaves: leaves}) | Empty
and leaves = {number: int, entries: array<tree>}

[cristianoc]

And a bigger question: the most time consuming part is going around fix all the references to the type.
Should an attempt be made to automate this? As currently we automate a simple copy-paste.

[cristianoc]

Notice we already have renaming working well across files, so fixing references might not be super difficult. Perhaps. One would have to try.

[cristianoc]

Just as a simple test.

Before:

type myType = This | That

let fun1 = (x: myType) => x

let fun2 = b => b ? This : That

Ohhh I was surprised to see that half of this is done automatically already:

module MyType = {
  type t = This | That
}

let fun1 = (x: MyType.t) => x

let fun2 = b => b ? This : That

What's left is the final step:

module MyType = {
  type t = This | That
}

let fun1 = (x: MyType.t) => x

let fun2 = b => b ? MyType.This : That

And one might or might not want to try to automate the final step.

[cristianoc]

Naming: should this be called "move type definition into its own module"?

[aspeddro]

Now support variant/record/object

type state = New | Unread | Read
type refState = state
type person = {"age": int, "name": string}
type user = {
  name: string,
  age: int,
}
and response = Yes | No

type myType = This | That

let fun1 = (x: myType) => x
let fun2 = b => b ? This : That
let fun3 = b => b ? {name: "Lhs", age: 2} : {name: "Rhs", age: 3}
let fun4 = b => b ? Yes : No
let me: person = {
  "age": 5,
  "name": "Big ReScript",
}

module State = {
  type t = New | Unread | Read
}
module RefState = {
  type t = State.t
}
module Person = {
  type t = {"age": int, "name": string}
}
module User = {
  type t = {
    name: string,
    age: int,
  }
  and response = Yes | No
}
module MyType = {
  type t = This | That
}

let fun1 = (x: MyType.t) => x
let fun2 = b => b ? MyType.This : That
let fun3 = b => b ? {User.name: "Lhs", age: 2} : {User.name: "Rhs", age: 3}
let fun4 = b => b ? User.Yes : No
let me: Person.t = {
  "age": 5,
  "name": "Big ReScript",
}

[cristianoc]

Looks like this is making steady progress.
Keep on going, and feel free to ping when more feedback is required.

[zth]

I haven't forgotten about the high level questions @cristianoc , will get back with them soon.

[zth]

Here are a few thoughts from me:

• I'm not worried about having too many code actions at this point. That's primarily because I think the way code actions are used is more about learning that certain actions exist in certain contexts, and then using the ones who fit the workflow you personally like. This might be a bit different if we end up having a lot of code actions, and I don't think we should have a bunch of borderline useless actions just for having actions. But at the point we're at now, with code actions in the extension being in its infancy, I think we can afford to experiment some. Even if that means removing things eventually.
• For this particular code action, I'd like to ask ourselves this - is this a code pattern we'd recommend (modules with a type t to encapsulate related types/functions etc)? Personally, I'm inclined to say yes. t is a pretty strong convention, and also what pipe autocompletion is based on. If this is a pattern we recommend, making it as easy as possible to work with is a goal imo. And for this particular pattern, one of the annoying things of moving a type to its own module is the refactoring part, which this action goes some (all?) of the way to solve.

Thoughts?

[cristianoc]

Agreed.
Just one small comment. Recommendation sounds a bit strong. In that you don't want a bureaucratic one module per type.
But, there are many cases where it makes sense.
So perhaps a bit more nuanced.

[zth]

Agreed. Just one small comment. Recommendation sounds a bit strong. In that you don't want a bureaucratic one module per type. But, there are many cases where it makes sense. So perhaps a bit more nuanced.

Yes, I agree. And I only really mean using t as the main type of a module. Anyways, with that said I think going ahead with this would be nice. Again, we can always call it experimental to start with.

[cristianoc]

Great. Let's move on with this.
Seems pretty much ready already.

[aspeddro]

It's ready for review.

[cristianoc]

This looks great.
Left some comments, nothing substantial.
There's a "ready for review" comment deleted so not sure if more changes are planned.

[cristianoc]

Feels like this logic should be factored out into its own easy to modify little local module.

[cristianoc]

Another instance of this.
If this does something useful, it should probably be moved into a standalone function inside Uri.

[cristianoc]

We have quite a few of these.
Need a little sanity check:

• that they don't do repeat work in a way that hurts perf (iterating several times is probably ok while copying would not)
• that no work is done unless the action fires

This is just about reviewing the code and check that things are OK.

[aspeddro]

It will take some time to get ready. There are more cases I need to check.