Skip to content

Expose version number#176

Open
rymdbar wants to merge 1 commit intobscan:mainfrom
rymdbar:expose-version-number
Open

Expose version number#176
rymdbar wants to merge 1 commit intobscan:mainfrom
rymdbar:expose-version-number

Conversation

@rymdbar
Copy link
Copy Markdown

@rymdbar rymdbar commented Nov 20, 2025

Fix #172.

Add serverInfo to LSP initialize method, and a --version option.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a user-visible version surface for Perl Navigator by exposing version/name via both the CLI and the LSP initialize response.

Changes:

  • Introduces a generator script (update-version.js) to produce server/src/perlnavigator.ts with NAME and VERSION constants from package.json.
  • Adds --version CLI handling and includes serverInfo (name/version) in the LSP InitializeResult.
  • Updates CI and ignores the generated server/src/perlnavigator.ts file.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
update-version.js Generates a TS module exporting NAME/VERSION from the root package.json.
server/src/server.ts Prints version on --version and reports serverInfo in LSP initialization.
package.json Runs the generator in ci-server.
.gitignore Ignores the generated server/src/perlnavigator.ts file.
Comments suppressed due to low confidence (1)

server/src/server.ts:50

  • The stdio fallback only triggers when process.argv.length <= 2. That means running perlnavigator -h/--help/-V (mentioned in #172) will still reach createConnection(...) without any transport flag and likely produce the same “connection input stream” error. Consider applying the fallback whenever no transport arg is present, or handling -h/--help/-V explicitly before initializing the connection.
// It the editor doesn't request node-ipc, use stdio instead. Make sure this runs before createConnection
if (process.argv.length <= 2) {
    process.argv.push("--stdio");
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread update-version.js
Comment on lines +2 to +10
const pkg = require("./package.json");

const content = `
export const VERSION = "${pkg.version}";
export const NAME = "${pkg.name}";
`;

fs.writeFileSync("server/src/perlnavigator.ts", content);
console.log("Generated server/src/perlnavigator.ts");
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

writeFileSync("server/src/perlnavigator.ts", ...) (and the require("./package.json") above) depend on the current working directory being the repo root. Using __dirname + path.join(...) for both input and output paths would prevent failures if the script is run from a different CWD (e.g., tooling/CI).

Suggested change
const pkg = require("./package.json");
const content = `
export const VERSION = "${pkg.version}";
export const NAME = "${pkg.name}";
`;
fs.writeFileSync("server/src/perlnavigator.ts", content);
console.log("Generated server/src/perlnavigator.ts");
const path = require("node:path");
const pkg = require(path.join(__dirname, "package.json"));
const content = `
export const VERSION = "${pkg.version}";
export const NAME = "${pkg.name}";
`;
const outputPath = path.join(__dirname, "server", "src", "perlnavigator.ts");
fs.writeFileSync(outputPath, content);
console.log(`Generated ${outputPath}`);

Copilot uses AI. Check for mistakes.
Comment thread server/src/server.ts
Comment on lines +40 to +41
import { VERSION, NAME } from "./perlnavigator";

Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

server.ts now imports ./perlnavigator, but server/src/perlnavigator.ts is generated (and gitignored) and does not exist in a fresh checkout. This will break TypeScript builds/webpack packaging unless version generation is wired into the normal build flow (e.g., pre-compile/package/watch/install-server) or the file is checked in with stable exports.

Suggested change
import { VERSION, NAME } from "./perlnavigator";
const DEFAULT_NAME = "Perl Navigator";
const DEFAULT_VERSION = "dev";
let NAME: string = DEFAULT_NAME;
let VERSION: string = DEFAULT_VERSION;
try {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const perlnavigator = require("./perlnavigator") as { NAME?: string; VERSION?: string };
if (perlnavigator.NAME) {
NAME = perlnavigator.NAME;
}
if (perlnavigator.VERSION) {
VERSION = perlnavigator.VERSION;
}
} catch {
// Fall back to default values when ./perlnavigator is not available.
}

Copilot uses AI. Check for mistakes.
Comment thread package.json
Comment on lines 386 to 388
"ci-client": "cd client && npm ci",
"ci-server": "cd server && npm ci",
"ci-server": "node update-version.js && cd server && npm ci",
"ci-all": "npm ci && npm run ci-client && npm run ci-server",
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Only ci-server runs update-version.js, but npm run compile/watch and the package/vscode:prepublish webpack build also compile server/src/server.ts. Without running the generator there, builds will fail due to the missing server/src/perlnavigator.ts module. Consider adding a dedicated generate-version step and invoking it from compile, watch, and package (and/or postinstall / install-server).

Copilot uses AI. Check for mistakes.
@bscan
Copy link
Copy Markdown
Owner

bscan commented Mar 21, 2026

Thanks for submitting the perl requests, and my apologies for the long delay in reviewing this. I'm not an expert on this type of packaging, so I kicked it over to copilot. I think the comment about adding a try/catch seems like the most important. If it's a fresh checkout, or some process didn't run the update-versions script (e.g. Github Actions), the language server should still work.

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.

Please consider exposing version string to user

3 participants