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

Fix windows test failures #311

Merged
merged 8 commits into from
Apr 13, 2024
Merged

Fix windows test failures #311

merged 8 commits into from
Apr 13, 2024

Conversation

Almenon
Copy link
Collaborator

@Almenon Almenon commented Feb 10, 2024

Pull Request

Related Github Issues

Description

  • Fix env var parsing bug
  • Add personal vscode workspace file to gitignore
  • Update readme

cmd/generate.go Outdated
@@ -31,7 +31,7 @@ func getEnvs() map[string]string {
m := make(map[string]string)

for _, env := range envs {
results := strings.SplitN(env, "=", 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

a small unittest for this would be nice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already have a test for this, check out the failing test for main branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dennislapchenko following up on this

Copy link
Collaborator

@pseudomorph pseudomorph Mar 5, 2024

Choose a reason for hiding this comment

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

Seems like this should be done with Cut, as per the docs.

https://pkg.go.dev/strings#SplitN

To split around the first instance of a separator, see Cut.

Perhaps something like:

- results := strings.SplitN(env, "=", 2)
- m[results[0]] = results[1]
+ varName, varValue, found := strings.Cut(env, "=")
+ if found {
+   m[varName] = m[varValue]
+ }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

either way works, let's just go with what's already in the code and passing tests

@pseudomorph
Copy link
Collaborator

pseudomorph commented Mar 21, 2024

@Almenon @dennislapchenko - Any update on this? This Blocks #320

@Almenon
Copy link
Collaborator Author

Almenon commented Mar 21, 2024

I'm waiting for a review from @dmattia or others, I don't have the ability to self-merge.

@Almenon Almenon changed the title Fix env var logic, windows test failures Fix windows test failures Mar 31, 2024
@Almenon
Copy link
Collaborator Author

Almenon commented Mar 31, 2024

It looks like David fixed the env var parsing bug in a commit directly to master, I guess they didn't see this PR. @pseudomorph if you rebase your tests should work now.

@dmattia can I get a review please? This PR used to fix the env var parsing bug and a windows bug. Now that you've fixed the first bug but this PR just fixes the windows bug, so it's a small baby-rabbit sized PR 🐰 .

Copy link
Collaborator

@pseudomorph pseudomorph left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Almenon Almenon merged commit cfa8c72 into master Apr 13, 2024
7 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix_env_var branch April 13, 2024 05:16
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.

3 participants