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

Adopt afero, create tests for globby. #1090

Merged
merged 8 commits into from
Apr 25, 2022
Merged

Conversation

nathanhammond
Copy link
Contributor

This PR does three things:

  • Adopt afero inside of the globby module. This is a nice isolated test of afero, which we can expand into the rest of the codebase if we remain happy with it.
  • Writes a bunch of tests of the globby implementation in order to attempt to specify the behavior prior to rewriting the implementation of GlobFiles.
  • Removes the implicit dependency on os.Getwd in one call to GlobFiles.

I've added a review for questions.

@vercel
Copy link

vercel bot commented Apr 21, 2022

@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

cli/internal/globby/globby.go Outdated Show resolved Hide resolved
cli/internal/globby/globby_test.go Outdated Show resolved Hide resolved
cli/internal/globby/globby_test.go Show resolved Hide resolved
cli/internal/globby/globby_test.go Outdated Show resolved Hide resolved
cli/internal/run/run.go Show resolved Hide resolved
@nathanhammond
Copy link
Contributor Author

The Windows CI failure is legitimately failing because of path separator differences. I expected them to be normalized and they ... weren't.

@vercel
Copy link

vercel bot commented Apr 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
turbo-site ✅ Ready (Inspect) Visit Preview Apr 25, 2022 at 6:24PM (UTC)

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

I'm super excited for this. I think this will be a major upgrade for the codebase.

cli/internal/globby/globby.go Outdated Show resolved Hide resolved
cli/internal/globby/globby.go Outdated Show resolved Hide resolved
cli/internal/globby/globby.go Show resolved Hide resolved
cli/internal/globby/globby_test.go Outdated Show resolved Hide resolved
cli/internal/globby/globby_test.go Outdated Show resolved Hide resolved
cli/internal/globby/globby_test.go Outdated Show resolved Hide resolved
cli/internal/globby/globby_test.go Show resolved Hide resolved
cli/internal/globby/globby.go Show resolved Hide resolved
cli/internal/run/run.go Show resolved Hide resolved
cli/internal/run/run.go Show resolved Hide resolved
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

One extra idiomatic Go thing, which I apologize for not catching earlier: you almost always want to use foo := thing instead of var foo = thing. The style guide covers it here

And actually, having now read that section, I suspect I am not 100% following it, specifically the piece about using var for empty slices. I'm not 100% sold on their argument, but if we're going to try to follow the guide, I guess we ought to move in that direction going forward.

Happy to have a good shared rant about Go sometime in an unrecorded medium.

@nathanhammond
Copy link
Contributor Author

I went back and addressed all the style guide issues I introduced here.

cli/internal/globby/globby.go Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit 84a3f80 into vercel:main Apr 25, 2022
@nathanhammond nathanhammond deleted the afero-globby branch April 26, 2022 03:26
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