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

give this project some more care #52

Merged
merged 16 commits into from
Jul 31, 2024
Merged

give this project some more care #52

merged 16 commits into from
Jul 31, 2024

Conversation

Techatrix
Copy link
Member

No clue what I should name this PR so please forgive me.
The individual commits should give a good overview of everything I have changed but here are some of the highlights:

  • add doc comments to the KnownFolder enum
  • add an installation guide in the README
  • port xdg-user-dir-lookup.c from xdg-user-dirs to Zig which takes care of properly parsing the user-dirs.dirs file.
  • add tests for linux, macos and wasm

I have abstracted away all OS interactions to test different environment variables and user-dirs.dirs files. It can also test different comptime configs (KnownFolderConfig).

The public API remains unchanged.

@Techatrix Techatrix force-pushed the dev branch 2 times, most recently from 0b903fe to a3f6185 Compare July 12, 2024 15:51
This switches from to `goto-bus-stop/setup-zig` to `mlugg/setup-zig`
This removes the arena allocator and prepares the code for the next commit that will also remove some allocations
…for testing purposes

This also allows us to `std.posix.getenv` instead of `std.process.getEnvVarOwned` on linux.
Thanks to the "System" abstraction, all possible environment variables and `user-dirs.dirs` can be tested.
gyro has been archived
@Techatrix
Copy link
Member Author

Anyone out here who would like to review this?

build.zig Show resolved Hide resolved
known-folders.zig Show resolved Hide resolved
@ikskuh
Copy link
Member

ikskuh commented Jul 27, 2024

Looks okay overall, but pretty large to review. I assume it works on your machine?

@Techatrix
Copy link
Member Author

Looks okay overall, but pretty large to review. I assume it works on your machine?

Yes, I have tested this locally on Linux. The windows specific code is mostly unchanged. The tests that I have added run for linux, macos and wasi. See 515e1f5. They even contain some macos specific tests that I never ran locally.

Should I split up this PR to make it easier to review? The whole diff looks very messy but I tried to make the individual commit readable as possible. With limited success. Please let me know if there is anything I can do to make this easier to review.

Copy link
Member

@ikskuh ikskuh 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 pretty happy with it as-is for :)

@ikskuh
Copy link
Member

ikskuh commented Jul 31, 2024

@Techatrix shall we merge?

@ikskuh ikskuh closed this Jul 31, 2024
@ikskuh ikskuh reopened this Jul 31, 2024
@Techatrix Techatrix merged commit 1cceeb7 into ziglibs:master Jul 31, 2024
10 checks passed
@Techatrix Techatrix deleted the dev branch July 31, 2024 15:21
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