-
Notifications
You must be signed in to change notification settings - Fork 91
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
Rewrite require and luaurc in lune-std #247
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I have not yet made comments on any specific files because I wanted to bring a few general points up. First, the positives:
- The improvements in error handling and the more strongly typed errors are great. I believe this to be the main highlight of this PR.
- The new library trait is a good change for flexbility and I hope we can also use it in the future for package management or something similar.
And then, a couple negatives:
- There are areas where this new implementation introduces TOCTOU race conditions, due to the heavy usage of
fs::try_exists
. It is very important that if a file read succeeds, we use the contents of the file at that specific moment in time. - Rewriting
LuauRc
fully and removing every field but thealiases
one may have been a step too far. It would be nice if Lune could help the user in case they have an invalid config and tell them what is wrong with their fields other than just the aliases. We just needed better error propagation.
Finally, some nitpicks. Feel free to ignore them and wait for me to comment on the specific files and locations if you wish:
- We can use the path utils from
lune_utils::path
instead of re-implementing new versions inlune-std
. - In many cases the new error propagation, which is great, introduces allocation when not necessary. Try to use
ok_or_else
instead ofok_or
when creating the error needs allocation.
Hey thanks for the feedback! I really appreciate it
Ive changed quite alot of code to get around this, it should be good now 😅
I had no idea why we we're de-serializing all those fields hence I didn't include them in the rewrite, I've bringed back all the fields into the struct so it should be good now (excluding the
I didn't know we had all those functions in lune_utils lol, I've removed all the duplicate functions and replaced them with the lune_utils path functions
Done! |
Highlights of this PR
.luaurc
if.luaurc
contains invalid JSON syntax #210)API Changes
for the existing crates that use
lune-std
nothing has to be changed, since this pr only changes the internal code and adds 2 new items which are totally optional and can be accessed by other crates, which are: