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

go embed fs with path #98

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

llonchj
Copy link
Contributor

@llonchj llonchj commented May 9, 2024

Before creating your Pull Request...

Go embed FS are

  • New Pull Requests should include a good description of what's being merged.
  • Ideally, all Pull Requests are preceded by a discussion initiated in an Issue on this repository.
  • For bug fixes is mandatory to have tests that cover and fail when the bug is present and will pass after this Pull Request.
  • For changes and improvements, new tests have to be provided to cover the new features.

Is this a fix, improvement or something else?

yes,

What does this change implement/fix?

  • allows to use go embed by adding a backwards compatible NewLocaleFSWithPath function,
  • adds tests for a embed.FS, addressing Allow locale to use a fs.FS #68
  • bumps go version to 1.16 which is the minimum required to use embed package

Bumping go version can be troublesome for existing users, although embed.FS tests will not be supported. It is possible to keep 1.13 compatibility by removing the additional unit test case.

Either way, in order to use embed.FS path seems to be required.

I have ...

@llonchj
Copy link
Contributor Author

llonchj commented May 9, 2024

@leonelquinteros what is the best case scenario for this PR?

  1. bump go version to 1.16, or
  2. remove the unittest for go:embed.

@leonelquinteros leonelquinteros merged commit be49c2f into leonelquinteros:master Sep 27, 2024
@leonelquinteros
Copy link
Owner

I'm OK with bumping Go version as these versions aren't even officially supported by the language: https://go.dev/doc/devel/release#policy

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.

2 participants