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.

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

Update version prompt if l2 binary below v1.5.1 #11

Merged
merged 11 commits into from
Jul 26, 2023

Conversation

lovestaco
Copy link
Collaborator

@lovestaco lovestaco commented Jul 23, 2023

What type of MR is this?

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Doc Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • ⏩ Revert

Description

  • This MR adds the feature of Adding a version checker for v1.5.1 using semver.
  • Update prompt for the l2 binary to the latest using l2 -u, prompt will come as a warning + description + button .
  • Adding Terminal functionalities in Utils and using it in Execute and Gen Snippet.

Mobile & Desktop Screenshots/Recordings

If there is no l2binary installed

image

If l2binary is less than v1.5.1

image

image

Important code file to start Code Review from

Check l2Version.ts

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Are there any post-deployment tasks we need to perform?

  • Need to prettify the codebase before merging.

- Added utils so next dev can use the teminal functionalities.
- Using getShowLama2Term() in generateCodeSnippet.ts and executeCurrentFile.ts
@lovestaco lovestaco changed the title Origin/update version prompt Update version prompt if l2 binary below v1.5.1 Jul 24, 2023
exec("l2 --version", (error, stdout, stderr) => {
const versionOutput = stdout.trim();
const versionMatch = versionOutput.match(/v(\d+\.\d+\.\d+)/);
if (versionMatch && versionMatch.length === 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why length === 2? We can use capture group to get a triple for (major, minor, patch) as well directly, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can try to use the official semver regex. See here for details. Check the actual regex in action at regex101

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Damn never knew so many kinds are there

});
}

function compareL2Versions(currentVersion: string, latestVersion: string) {
Copy link
Contributor

@shrsv shrsv Jul 24, 2023

Choose a reason for hiding this comment

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

I recommend using a library suggested in this SO Answer. Our semver definition is simple now, but there are more changes possible. Best to simply use standardized library for it.

Should be as simple as:

semver.gte('3.4.8', '3.4.7') //true

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using this library, then ignore the previous comment about regex*.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah using this

@shrsv
Copy link
Contributor

shrsv commented Jul 24, 2023

Just curious - have we already integrated the variable suggestion logic (from both l2.env and l2config.env)? If not - we can do it in this MR itself.

src/extension.ts Outdated
@@ -34,6 +35,9 @@ export function activate(context: vscode.ExtensionContext) {
context.subscriptions.push(generateCodeSnippetDisposable);
console.log(">>> generateCodeSnippetDisposable is now active!");

// Automatically check L2 version on extension activation
checkL2Version();
Copy link
Contributor

Choose a reason for hiding this comment

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

So if version is too old, what happens to the variable suggestion? Is it totally dead in older versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No in the older version what we were doing is searching the le.env file in front end and suggesting it. So it won't be a problem.

Copy link
Contributor

@shrsv shrsv Jul 24, 2023

Choose a reason for hiding this comment

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

What I am saying is something else.

Say, the extension frontend updates, but the l2 version is still old, what happens?

Remember - VSCode automatically updates extensions quite quickly (I think)

@lovestaco
Copy link
Collaborator Author

Just curious - have we already integrated the variable suggestion logic (from both l2.env and l2config.env)? If not - we can do it in this MR itself.

Yes its integrated to main but havent released it yet

#10

const LAMA2_TERM_NAME = "AutoLama2";
const UPDATE_MSG = "Support for environment variables.";

export function checkL2Version() {
export async function getL2VersionAndUpdatePrompt(minVersionToCheck: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Which function call within this requires async? I don't see any await

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this, it wasn't needed

});
return normalizedVersion;
} else {
throw new Error("Invalid version format: " + l2Version);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we report to the user - that the version is unknown and probably the extension will not work in full?

We can have a button "Download" to direct them to website.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this

return normalizedVersion;
} else {
throw new Error("Invalid version format: " + l2Version);
}
} catch (e: any) {
console.log("Problem while checking for version -> ", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should surface this as a warning - I think.

Especially - what happens when there is no binary in the path?

I think the user ought to know that something is wrong, and that action is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah added

@shrsv
Copy link
Contributor

shrsv commented Jul 26, 2023

LGTM

@shrsv shrsv merged commit 0d33a12 into main Jul 26, 2023
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.

None yet

2 participants