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

Auto suggest environment variables using l2 --env <path> command #10

Merged
merged 9 commits into from
Jul 18, 2023

Conversation

lovestaco
Copy link
Collaborator

@lovestaco lovestaco commented Jul 16, 2023

What type of MR is this?

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

Description

Edit1:
Merging old copygit branch autosuggest env feature.

Edit2:
This MR adds the feature of suggesting root and local variables.

  • Using the new option for l2command l2 --env <l2FilePath>.
  • The l2 go binary will output a JSON of environment variables with metadata from l2config.env and l2.env.
  • Added this feature so VScode reads the JSON and suggests variables in a quicker and more detailed high-quality way.

Important code file to start Code Review from

extention.ts

Related PR

Global variable support and Generate env JSON option

Added tests?

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

rohansh-tty and others added 7 commits March 5, 2023 15:52
    - Handling this case in catch block
- Using `l2 --env ${l2FilePath}` go lang binary command to fetch env.
- Removed old searching envs in the frontend as performance was low.
- Moved all the suggestions and replacing part to new file.
@lovestaco lovestaco changed the title Auto suggest environment variables from l2.env Auto suggest environment variables using l2 --env command Jul 16, 2023
@lovestaco lovestaco changed the title Auto suggest environment variables using l2 --env command Auto suggest environment variables using l2 --env <path> command Jul 16, 2023
@shrsv
Copy link
Contributor

shrsv commented Jul 16, 2023

This MR adds the feature of suggesting global and local variables

In documentation, we should settle with a proper name - something like root variables or project variables. Just pick one of these two and use everywhere. Consistency is key, to reduce ambiguity.

src/extension.ts Outdated

// Level1 command pallette
function isDefault(defaultClient: string, client: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this function about? not able to infer from context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should go into a group of functions in a different file. My best guess is it's related to the code generation library selection.

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 moved everything to new files.

src/extension.ts Outdated
},
}
);
let suggestEnvVariables = suggestENVS();
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestENVs


}

export function suggestENVS() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ENVs

// const commandOutput = execSync(`./build/l2 --env ${l2FilePath}`, { cwd: "/home/lovestaco/repos/Lama2", }).toString(); // For local debugging
const commandOutput = execSync(`l2 --env ${l2FilePath}`).toString();

const mapStartIndex = commandOutput.indexOf("{");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we don't want to rely on this. Say, if a logger has a { character, this is going to break. So, if at all possible we have a full quiet mode in the command output itself, so that there's no need to find the map's starting point. We should simply be able to do JSON.parse on incoming string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes made the change

@lovestaco lovestaco self-assigned this Jul 16, 2023
@shrsv shrsv merged commit 975913b into main Jul 18, 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

3 participants