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

Use correct tuple ordering in resolve_paths #171

Conversation

kennethloeffler
Copy link
Contributor

@kennethloeffler kennethloeffler commented Apr 6, 2024

After noticing that the test in #170 failed, and after some inspection in a debugger, I discovered some incorrectness around the RequireContext::resolve_paths method. This method returns a tuple (rel_path, abs_path), but the caller seems to expect (abs_path, rel_path):

pub fn resolve_paths(
&self,
source: impl AsRef<str>,
path: impl AsRef<str>,
) -> LuaResult<(PathBuf, PathBuf)> {
let path = PathBuf::from(source.as_ref())
.parent()
.ok_or_else(|| LuaError::runtime("Failed to get parent path of source"))?
.join(path.as_ref());
let rel_path = path_clean::clean(path);
let abs_path = if rel_path.is_absolute() {
rel_path.to_path_buf()
} else {
CWD.join(&rel_path)
};
Ok((rel_path, abs_path))
}

let (abs_path, rel_path) = ctx.resolve_paths(source, path)?;

This PR closes #138 by swapping the order of the tuple fields in RequireContext::resolve_paths to match the rest of the code.

Copy link
Contributor

@vocksel vocksel 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 getting this PR up. Looks good on my end though I can't approve, just chiming in with an extra set of eyes. One minor suggestion to fix a typo but other than that looks great 👍

tests/require/tests/state.luau Outdated Show resolved Hide resolved
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

Nice catch, I'm a bit surprised that things were working in the first place with these being swapped, and that require caching was the only issue 😅

And thanks to @itsfrank for the tests to make sure it doesn't happen again!

@filiptibell filiptibell merged commit a65ef2a into lune-org:main Apr 7, 2024
4 of 5 checks passed
@kennethloeffler kennethloeffler deleted the fix-resolve-paths-swapped-tuple-fields branch April 7, 2024 19:49
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.

Lune does not cache require results across files
4 participants