The path option was added to the rescript.settings #478

An-Tu · 2022-07-05T23:14:33Z

This PR fixes #412 .

I added the handling of the relative path and also the path with contains the home directory symbol. But I'm not sure that it is ok.

An-Tu · 2022-07-05T23:50:10Z

It seems that I missed. I thought that the relative paths handling is necessary but I double-checked and it looks like not. I removed the extra code.

cristianoc

Thanks for working on this.
Was this PR tested?

server/src/server.ts

server/src/utils.ts

amiralies · 2022-07-06T00:14:37Z

I suggest to use a more descriptive name for path. path is very generic something like pathToRescript rescriptPath is better IMO.

An-Tu · 2022-07-06T00:41:39Z

I suggest to use a more descriptive name for path. path is very generic something like pathToRescript rescriptPath is better IMO.

It will look like "rescript.settings.path": "/some/path" in settings. It doesn't seem very generic. Or do you think that there will be more settings in the future that can contain other paths?

An-Tu · 2022-07-06T00:44:27Z

Was this PR tested?

Yes, I tested manually.

cristianoc · 2022-07-06T00:53:26Z

Perhaps binaryPsth ?

amiralies · 2022-07-07T02:16:38Z

The thing is path itself doesn't show what it actually means.
is this path to rescript binary? path to a custom analysis binary or even project root path?
some other extensions i've used also use more descriptive or scoped names.
for example iirc rust-analyzer uses rust-analyzer.server.path (path to language server) and flow uses pathToFlow.

An-Tu · 2022-07-07T21:06:59Z

I changed path to binaryPath

package.json

server/src/server.ts

cristianoc

I was wondering whether this can possibly affect the existing way of determining how the path is computed.
I could not answer the question as there are a dozen changes happening at the same time.

If there's a way to change less, then it's easier to review.

If there isn't then this needs to be broken up into several small refactoring PRs where each one is obviously correct.

…was added.

An-Tu · 2022-07-10T21:09:04Z

@cristianoc I have divided all changes into commits which should be easy to check. But if it is not suitable please tell me and I will prepare the separate PRs.

cristianoc · 2022-07-11T03:21:13Z

@cristianoc I have divided all changes into commits which should be easy to check. But if it is not suitable please tell me and I will prepare the separate PRs.

This format is perfect thanks. Let me take a look.

cristianoc · 2022-07-11T03:25:24Z

server/src/server.ts

@@ -596,7 +596,11 @@ function format(msg: p.RequestMessage): Array<p.Message> {
  } else {
    // code will always be defined here, even though technically it can be undefined
    let code = getOpenedFileContent(params.textDocument.uri);
-    let formattedResult = utils.formatCode(filePath, code);
+    let formattedResult = utils.formatCode(
+      extensionConfiguration.binaryPath,


I don't understand this logic. The fact that there exists a configuration, which is optional, should not show up here.
The logic should be: find the binary path and use it. Either here or directly in the utils command.

cristianoc · 2022-07-11T03:27:59Z

server/src/utils.ts

  let extension = path.extname(filePath);
  let formatTempFileFullPath = createFileInTempDir(extension);
  fs.writeFileSync(formatTempFileFullPath, code, {
    encoding: "utf-8",
  });
  try {
+    // Try to find the bsc bin from the binaryPath setting from the configuration.
+    let bscPath = findBscBinFromConfig(pathToBinFromConfig);


This logic is a bit strange. Seems biased towards the configuration. While the configuration is the exception not the rule.

The logic I would expect is:

find the normal binary path

if the config has a setting use that instead

cristianoc · 2022-07-11T03:29:14Z

server/src/utils.ts

-    if (bscNativePath != null) {
-      let result = childProcess.execFileSync(bscNativePath, [
+    // Default to using the formatter from the binaryPath setting from the configuration
+    // or the project formatter.


This comment is also inverted.
Default to use the binary path that ships with the analysis binary.
But if there is one in the config, override.

This is only about how the code reads, the semantics is the same.

cristianoc · 2022-07-11T03:31:57Z

server/src/server.ts

@@ -65,6 +65,11 @@ let codeActionsFromDiagnostics: codeActions.filesCodeActions = {};
 // will be properly defined later depending on the mode (stdio/node-rpc)
 let send: (msg: p.Message) => void = (_) => {};

+let findBinary = (projectRootPath: p.DocumentUri) =>


Which binary?
findBuildBinary.

All these functions, including findNodeBuildOfProjectRoot need to be renamed to contain the name Build.
So findBuildBinaryFromProjectRoot and findBuildBinaryFromConfig.

cristianoc · 2022-07-11T03:37:29Z

server/src/utils.ts

-    return { buildPath: rescriptNodePath, isReScript: true };
-  } else if (fs.existsSync(bsbNodePath)) {
-    return { buildPath: bsbNodePath, isReScript: false };
+let findBinaryBase = ({


Add a comment to explain what this does.

cristianoc · 2022-07-11T03:41:09Z

server/src/server.ts

@@ -262,7 +262,20 @@ let openedFile = (fileUri: string, fileContent: string) => {
        // handle in the isResponseMessage check in the message handling way
        // below
      } else {
-        // we should send something to say that we can't find bsb.exe. But right now we'll silently not do anything
+        let binaryPath =


Isn't there already a function that does this: finding the binary directory?

cristianoc · 2022-07-11T03:42:11Z

I made a few reviews, going commit after commit.
The semantics seems fine. All the comments are about naming and clarity.

cristianoc · 2022-07-11T03:54:47Z

Overall I think this should be the main logic:

There's a concept of binary path, that's the path where binaries are found.
There's a concept of finding specific binaries.
There's a list of binaries, probably a variant.
There are functions to obtain specific binaries.

The config would be mentioned only once in one place: find the binary path.

An-Tu · 2022-07-11T15:49:48Z

Thanks a lot for your review! In the near future, I will try to polish this PR.

…ction

An-Tu · 2022-07-12T10:23:20Z

@cristianoc I fixed the PR according to your reviews. Also, I have a few thoughts above. Could you please look at them?

cristianoc · 2022-07-12T14:22:24Z

Thank you -- merging now as I need to do a couple of relevant changes.

cristianoc reviewed Jul 6, 2022

View reviewed changes

server/src/server.ts Outdated Show resolved Hide resolved

server/src/utils.ts Outdated Show resolved Hide resolved

server/src/utils.ts Show resolved Hide resolved

An-Tu force-pushed the path_to_config branch from 2a7e73c to 79185cc Compare July 7, 2022 21:04

cristianoc reviewed Jul 8, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

server/src/server.ts Outdated Show resolved Hide resolved

server/src/server.ts Outdated Show resolved Hide resolved

An-Tu force-pushed the path_to_config branch 2 times, most recently from ea8c7a9 to f3eb068 Compare July 8, 2022 17:06

cristianoc reviewed Jul 9, 2022

View reviewed changes

An-Tu added 6 commits July 10, 2022 22:26

code formatting

81aaa11

binaryPath was added to the settings

7e21142

The handling of the paths which start with the home directory symbol …

4deba1e

…was added.

The binaryPath setting was added to the formatCode function

8108098

The binaryPath setting is using to find resctipt binary

6828f96

The notification about the rescript binary absent was added

505d3cb

An-Tu force-pushed the path_to_config branch from f3eb068 to 505d3cb Compare July 10, 2022 21:02

cristianoc approved these changes Jul 11, 2022

View reviewed changes

cristianoc reviewed Jul 11, 2022

View reviewed changes

An-Tu added 2 commits July 12, 2022 10:03

Fix findBuildBinary naming and fix comments

99e9f19

The search for the path to the bsc is taken out of the formatCode fun…

1589700

…ction

Fix notification about buildBinary absent

b6cfb98

cristianoc merged commit 8adf920 into rescript-lang:master Jul 12, 2022

The path option was added to the rescript.settings #478

The path option was added to the rescript.settings #478

Uh oh!

Conversation

An-Tu commented Jul 5, 2022

Uh oh!

An-Tu commented Jul 5, 2022

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amiralies commented Jul 6, 2022

Uh oh!

An-Tu commented Jul 6, 2022

Uh oh!

An-Tu commented Jul 6, 2022

Uh oh!

cristianoc commented Jul 6, 2022

Uh oh!

amiralies commented Jul 7, 2022

Uh oh!

An-Tu commented Jul 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

An-Tu commented Jul 10, 2022

Uh oh!

cristianoc commented Jul 11, 2022

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Jul 11, 2022

Uh oh!

cristianoc commented Jul 11, 2022

Uh oh!

An-Tu commented Jul 11, 2022

Uh oh!

An-Tu commented Jul 12, 2022

Uh oh!

cristianoc commented Jul 12, 2022

Uh oh!

Uh oh!