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.

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

refactor: Do not use .metals for checking the metals version #1076

Merged
merged 1 commit into from
Sep 1, 2022
Merged

refactor: Do not use .metals for checking the metals version #1076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ const decorationType: TextEditorDecorationType =
const config = workspace.getConfiguration("metals");

export async function activate(context: ExtensionContext): Promise<void> {
const serverVersion = getServerVersion(config);
const serverVersion = getServerVersion(config, context);
detectLaunchConfigurationChanges();
configureSettingsDefaults();

Expand Down
91 changes: 52 additions & 39 deletions src/getServerVersion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,39 @@
import {
commands,
ConfigurationTarget,
ExtensionContext,
window,
WorkspaceConfiguration,
} from "vscode";
import * as metalsLanguageClient from "metals-languageclient";
import * as workbenchCommands from "./workbenchCommands";
import http from "https";
import path from "path";
import fs from "fs";
import { getConfigValue, metalsDir } from "./util";
import { getConfigValue } from "./util";

const serverVersionSection = "serverVersion";
const suggestLatestUpgrade = "suggestLatestUpgrade";
const versionDatesFileName = "versions-meta.json";

export function getServerVersion(config: WorkspaceConfiguration): string {
const currentVersionKey = "currentVersion";
const lastUpdatedAtKey = "lastUpdatedAt";

export function getServerVersion(
config: WorkspaceConfiguration,
context: ExtensionContext
): string {
/* eslint-disable @typescript-eslint/no-non-null-assertion */
const serverVersionConfig = config.get<string>(serverVersionSection);
const defaultServerVersion =
config.inspect<string>(serverVersionSection)!.defaultValue!;
const serverVersion = serverVersionConfig?.trim() || defaultServerVersion;

validateCurrentVersion(serverVersion, config);
validateCurrentVersion(serverVersion, config, context);
return serverVersion;
}

async function validateCurrentVersion(
serverVersion: string,
config: WorkspaceConfiguration
config: WorkspaceConfiguration,
context: ExtensionContext
): Promise<void> {
const suggestUpgradeSetting = getConfigValue<boolean>(
config,
Expand All @@ -37,7 +42,11 @@ async function validateCurrentVersion(

const checkForUpdate = async () => {
if (suggestUpgradeSetting?.value) {
return needCheckForUpdates(serverVersion, suggestUpgradeSetting.target);
return needCheckForUpdates(
serverVersion,
suggestUpgradeSetting.target,
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, depending on where user set checkForUpdates switch then version will be stored in that scope (global vs workspace), I'm right?
Here, because it's a simple boolean there are no such cases like with server version itself (where empty string in workspace is treated as a valid value and global setting is discarded)

Copy link
Member Author

@tanishiking tanishiking Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand correctly, depending on where user set checkForUpdates switch then version will be stored in that scope (global vs workspace), I'm right?

Yes, you're right!

Here, because it's a simple boolean there are no such cases like with server version itself

What do you mean?
This is the same behavior with the previous one.

Do you mean, we should check for updating both Global and Workspace settings, even if suggetUpgradeSetting is true only in Global or Workspace?
In that case, I agree, but I think we can work on that in another issue / PR #1071

Copy link
Member

@kpodsiad kpodsiad Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@tanishiking I honestly don't remember what I was thinking and what I meant 😅 (even after reading my comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

haha, no worries! sorry for my delayed reply

context
);
} else {
return false;
}
Expand All @@ -59,10 +68,14 @@ async function validateCurrentVersion(
nextVersion,
suggestUpgradeSetting.target
);
saveVersionDate(nextVersion, suggestUpgradeSetting.target);
saveVersionDate(nextVersion, suggestUpgradeSetting.target, context);
} else if (result == ignoreChoice) {
// extend the current version expiration date
saveVersionDate(serverVersion, suggestUpgradeSetting.target);
saveVersionDate(
serverVersion,
suggestUpgradeSetting.target,
context
);
}
});
}
Expand All @@ -88,44 +101,44 @@ async function fetchLatest(): Promise<string> {
return sorted[sorted.length - 1];
}

/* The logic is the following:
- if version was set more than a day ago - update is needed
- if version is seen in a first time (user changed version in config by it self) - the update will be delayed for a day
/**
* The logic is the following:
* - if version was set more than a day ago - update is needed
* - if version is seen in a first time (user changed version in config by it self) - the update will be delayed for a day
*/
async function needCheckForUpdates(
currentVersion: string,
target: ConfigurationTarget
target: ConfigurationTarget,
context: ExtensionContext
): Promise<boolean> {
const file = path.join(metalsDir(target), versionDatesFileName);

const records: Record<string, string> = (() => {
if (!fs.existsSync(file)) {
return {};
} else {
const data = fs.readFileSync(file, { encoding: "utf8", flag: "r" });
return JSON.parse(data);
}
})();
const state =
target === ConfigurationTarget.Global
? context.globalState
: context.workspaceState;
const prevVersion = state.get<string>(currentVersionKey);
const lastUpdated = state.get<string>(lastUpdatedAtKey);

if (records[currentVersion]) {
return records[currentVersion] != todayString();
} else {
saveVersionDate(currentVersion, target);
const today = todayString();
if (prevVersion !== currentVersion) {
saveVersionDate(currentVersion, target, context);
return false;
} else {
return lastUpdated !== today;
}
}

function saveVersionDate(version: string, target: ConfigurationTarget): void {
const datesValue: Record<string, string> = {};
datesValue[version] = todayString();
const dir = metalsDir(target);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
fs.writeFileSync(
path.join(dir, versionDatesFileName),
JSON.stringify(datesValue)
);
function saveVersionDate(
version: string,
target: ConfigurationTarget,
context: ExtensionContext
): void {
const state =
target === ConfigurationTarget.Global
? context.globalState
: context.workspaceState;

state.update(currentVersionKey, version);
state.update(lastUpdatedAtKey, todayString());
}

function warnIfIsOutdated(config: WorkspaceConfiguration): void {
Expand Down