chore(file checker): separate resolving and checking for easier testing#1830
chore(file checker): separate resolving and checking for easier testing#1830mre merged 5 commits intolycheeverse:masterfrom
Conversation
this does a simple refactor to break up the `check_path` function. previously, this had the responsibility of resolving index files/fallback extensions _and_ checking fragments. this was done in one step which made it hard to test - in order to test the resolving, we had to test it indirectly by checking fragments in certain files. this introduces a new `resolve_local_path` function which only applies the index file + extensions, so we can use this in unit tests and make assertions about the resolved path. this should have no changes to any behaviour.
this makes the meaning of the tests a bit clearer. rather than going through fragments, we can directly make assertions about the resolved file path.
| Ok(meta) if meta.is_dir() => self.apply_index_files(path).map(Cow::Owned), | ||
| Ok(meta) if meta.is_dir() => self.apply_index_files(path), | ||
|
|
||
| // otherwise, path is an existing file - just return the path |
There was a problem hiding this comment.
Have you tried to return a Path (with a lifetime)? This would technically be a breaking change, but that should be fine if we can avoid the copy.
There was a problem hiding this comment.
Return a &Path with lifetime from resolve_local_path? I haven't tried, but my intuition was it wouldn't work because we can't return a reference to locally owned data. The owned data would get deallocated upon leaving the function.
Previously, Cow was used to hold either a owned or borrowed value, but I got rid of Cow because it was somewhat inconvenient to work with. I could add it back if you think that's a reasonable solution.
There was a problem hiding this comment.
Silly me, you're right.
I played around with it and created rina-forks#1. Can you take a look? I can see advantages and disadvantages on both sides, so I'm okay with keeping your version as well. 😉 If you decide to merge my PR, this PR should automatically get updated. 🪄
There was a problem hiding this comment.
It is merged, thanks for your help!
|
Yay, that's a solid improvement. Thanks @katrinafyi. 🙏 |
2 participants
this does a simple refactor to break up the
check_pathfunction. previously, this had the responsibility of resolving index files/fallback extensions and checking fragments. this was done in one step which made it hard to test - in order to test the resolving, we had to test it indirectly by checking fragments in certain files.this introduces a new
resolve_local_pathfunction which only applies the index file + extensions, so we can use this in unit tests and make assertions about the resolved path.this should have no changes to any behaviour.
please note perf bug.also, idk if I go overboard with the macros ;-;