Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
81aaa11
code formatting
An-Tu Jul 10, 2022
7e21142
binaryPath was added to the settings
An-Tu Jul 10, 2022
4deba1e
The handling of the paths which start with the home directory symbol …
An-Tu Jul 10, 2022
8108098
The binaryPath setting was added to the formatCode function
An-Tu Jul 10, 2022
6828f96
The binaryPath setting is using to find resctipt binary
An-Tu Jul 10, 2022
505d3cb
The notification about the rescript binary absent was added
An-Tu Jul 10, 2022
99e9f19
Fix findBuildBinary naming and fix comments
An-Tu Jul 12, 2022
1589700
The search for the path to the bsc is taken out of the formatCode fun…
An-Tu Jul 12, 2022
b6cfb98
Fix notification about buildBinary absent
An-Tu Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ export let analysisProdPath = path.join(
"rescript-editor-analysis.exe"
);

export let bscBinName = "bsc";

// can't use the native bsb/rescript since we might need the watcher -w flag, which is only in the JS wrapper
export let rescriptNodePartialPath = path.join(
"node_modules",
Expand Down
6 changes: 5 additions & 1 deletion server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

filePath,
code
);
if (formattedResult.kind === "success") {
let max = code.length;
let result: p.TextEdit[] = [
Expand Down
34 changes: 28 additions & 6 deletions server/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ export let findBscNativeOfFile = (
}
};

let findBscBinFromConfig = (
pathToBinFromConfig: p.DocumentUri | null
): null | p.DocumentUri => {
if (pathToBinFromConfig === null) {
return null;
}
let bscPath = path.join(pathToBinFromConfig, c.bscBinName);
if (fs.existsSync(bscPath)) {
return bscPath;
}
return null;
};

// TODO: this doesn't handle file:/// scheme
export let findNodeBuildOfProjectRoot = (
projectRootPath: p.DocumentUri
Expand All @@ -94,21 +107,30 @@ type execResult =
kind: "error";
error: string;
};
export let formatCode = (filePath: string, code: string): execResult => {

export let formatCode = (
pathToBinFromConfig: p.DocumentUri | null,
filePath: string,
code: string
): execResult => {
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


// See comment on findBscNativeDirOfFile for why we need
// to recursively search for bsc.exe upward
let bscNativePath = findBscNativeOfFile(filePath);
bscPath = bscPath == null ? findBscNativeOfFile(filePath) : bscPath;

// Default to using the project formatter. If not, use the one we ship with
// the analysis binary in the extension itself.
if (bscNativePath != null) {
let result = childProcess.execFileSync(bscNativePath, [
// Default to using the formatter from the binaryPath setting from the configuration
// or the project formatter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

// If not, use the one we ship with the analysis binary in the extension itself.
if (bscPath != null) {
let result = childProcess.execFileSync(bscPath, [
"-color",
"never",
"-format",
Expand Down