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

filemapping: add support for io/fs (including e.g. go:embed filesystems) #16

Merged
merged 6 commits into from
Jul 21, 2023

Conversation

moqmar
Copy link

@moqmar moqmar commented Jul 17, 2023

This allows to e.g. use //go:embed locales to build the locales into the binary.

Copy link

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I wonder if you could add some unit test (also to see how this will be used in practice ). I wonder for example if "LocalFS" should be exported or part of some New* helper and seeing how you use it or envision to use it would be helpful here :)

@moqmar
Copy link
Author

moqmar commented Jul 19, 2023

I think LocaleFS & LocalePath belong really closely together, I don't see why one would be exported and the other not, so no need to change the API IMO.

I've added the FS usage to the example with go:embed to the README.

Copy link

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for the test and the README.md addition. This looks very nice. Fwiw, I think it's fine if we bump from go-1.13 to go-1.18 in go.mod if you want to do a test that tests the go:embed behavior

filemapping_unix.go Outdated Show resolved Hide resolved
@moqmar
Copy link
Author

moqmar commented Jul 20, 2023

All done 👍

Edit: now finally all tests should pass, I obviously have a linux machine :D

@mvo5 mvo5 merged commit 9082cdc into snapcore:master Jul 21, 2023
3 checks passed
@mvo5
Copy link

mvo5 commented Jul 21, 2023

Thank you!

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