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

Write Go files with an internal helper instead of gofiles.Write #222

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tabboud
Copy link

@tabboud tabboud commented Oct 14, 2022

Before this PR

A break (no more template rendering) and a behavior change (using packages.Load to get module paths instead of $GOPATH) in nmiyake/pkg/gofiles caused tests to fail on palantir/godel-okgo-asset-importalias#132 since we're no longer writing go.mod files.

All usages of gofiles.Write did not inspect the returned import paths, so instead of using that function, we can use an internal function to write out the raw Go files avoiding any package loading and only returning errors for writing the files.

This is almost identical to what was done for distgo here: palantir/distgo#237

After this PR

==COMMIT_MSG==
Write Go files with an internal helper instead of gofiles.Write
==COMMIT_MSG==

Possible downsides?

Copy link
Contributor

@nmiyake nmiyake left a comment

Choose a reason for hiding this comment

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

Based on the fact that this is only file that changes, it seems like this change alone doesn't remove the dependency for gofiles -- is that correct? It feels like this change would make more sense if it removed the gofiles dependency from this module (if not fully/transitively, then at least for direct reference)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants