Skip to content

Conversation

@Magd74NA
Copy link

@Magd74NA Magd74NA commented Jun 27, 2025

Added a header file to represent simplifile's file_info type and rewrote the file type checking functions to take advantage of it.

Magd added 4 commits June 26, 2025 23:15
…erl based on the file.hrl from the erlang standard library
…able and idiomatic (I think?) using the file.hrl and simplifile_file_info.hrl records.
…taking advantage of the new record I wrote to make code more concise and idiomatic (I think?)
@Magd74NA
Copy link
Author

Hi! This is my first Github contribution ever and my work in Gleam and Erlang ever! I was reading the source code for simplifile to learn some Gleam and specifically about how the FFI to Erlang works which lead me to reading the Erlang file documentation and realizing there are some slightly nicer coding patterns (I think?) that could be used on the Erlang side by writing header files.

@bcpeinhardt
Copy link
Owner

Howdy! Thanks for making this PR!

I'm not exactly a pro erlanger either, so take my feedback with a grain of salt :)

I'm not overly fond of the check_file_type helper function, as I think it makes the control flow less clear to avoid a relatively small amount of boilerplate. Wherever possibly I want the ffi to be simple and shallow.

I'm also not a big fan of the introduction of the simplifile_file_info record type. I think it would probably be a great idea if we were just writing Erlang, but we already define the type it's representing in Gleam as the FileInfo type, and I'd prefer to have a single source of truth for the type's structure to avoid accidental drift.

I very much like the refactoring of file_info_result to take advantage of record syntax. Perhaps we could also add a comment linking to the file_info record's definition: https://www.erlang.org/doc/apps/kernel/file.html#t:file_info/0.

This is really good stuff for a first time with Gleam and Erlang 😎 I'm not sure simplifile is the best library to contribute to for the purpose of learning gleam, as it is heavily ffi based. That said the contribution is still very much welcome :)

@Magd74NA
Copy link
Author

Magd74NA commented Jun 27, 2025

Howdy! Thanks for making this PR!

I'm not exactly a pro erlanger either, so take my feedback with a grain of salt :)

I'm not overly fond of the check_file_type helper function, as I think it makes the control flow less clear to avoid a relatively small amount of boilerplate. Wherever possibly I want the ffi to be simple and shallow.

I'm also not a big fan of the introduction of the simplifile_file_info record type. I think it would probably be a great idea if we were just writing Erlang, but we already define the type it's representing in Gleam as the FileInfo type, and I'd prefer to have a single source of truth for the type's structure to avoid accidental drift.

I very much like the refactoring of file_info_result to take advantage of record syntax. Perhaps we could also add a comment linking to the file_info record's definition: https://www.erlang.org/doc/apps/kernel/file.html#t:file_info/0.

This is really good stuff for a first time with Gleam and Erlang 😎 I'm not sure simplifile is the best library to contribute to for the purpose of learning gleam, as it is heavily ffi based. That said the contribution is still very much welcome :)

Thank you very much for the feedback! I'm interested in the whole BEAM ecosystem in general really. Also I did specifically want to understand how to write FFI in Gleam. Gleam just seems like the cleanest and simplest to actually write things in. I've never done FP before so I'm still trying to wrap my head around everything involved. I found myself using the library a lot for my small practice projects so I wanted to understand it a bit better! I will update my PR with your suggestions.

Magd added 3 commits June 27, 2025 13:19
…nctions taking advantage of the new record I wrote to make code more concise and idiomatic (I think?)"

This reverts commit 767245a.
@Magd74NA
Copy link
Author

I just amended my PR with the changes you suggested.

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