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

Replace packr with stdlib embed #101

Merged
merged 4 commits into from
Jun 13, 2022

Conversation

philippgille
Copy link
Contributor

@philippgille philippgille commented Jun 11, 2022

This PR replaces github.com/gobuffalo/packr/v2 with Go standard library's embed.

It closes #85.

Main changes:

  • Rename directories with test data to testdata
    • following Go best practices (testdata directories have a special meaning in that the Go toolchain ignores files in it)
  • Move test data to their respective packages
    • Packr allows embedding of files in directories outside of the current file tree, but embed doesn't (e.g. ../testdata doesn't work)
  • Move known schemas to a central place, schema package
    • Because of the mentioned limitation (embed doesn't allow to embed from directories outside the current file tree), and to avoid duplicating all schema files
    • All packages using the known schema files already import the schema package, so there's no additional imports required
  • Remove packr related targets and code from the magefile

Currently it exposes one new public API, schema.GetKnownSchema, so that the packages importing schema can get the files as well. But alternatively we can move the known schemas and that GetKnownSchema method into a new internal/schema or internal/known_schemas package. internal packages are treated special by Go, in that all packages in this repo can access that package, but it won't be part of the public API anymore. I think this makes sense, but wanted to keep it simple for now, to have a first basis for discussing the PR. Please let me know what you think about this.

⚠️ Starting with the state of the main on commit 6e6b96e, without any code changes, running go mod tidy with Go 1.17 changed the go.mod and go.sum files. I guess it was forgotten to be run recently? In any case I included that as first commit in this PR.

Tested: mage cbd succeeds 🟢

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

perfect! thanks for taking this 😄

@decentralgabe decentralgabe merged commit cb99098 into TBD54566975:main Jun 13, 2022
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.

Switch Packr to Embed
2 participants