itb-community / ITB-ModLoader

A lua-based mod loader for the game Into the Breach
48 stars 18 forks source link

Add `Directory:relativize` and `Directory:is_ancestor` #181

Closed kartoFlane closed 1 year ago

kartoFlane commented 2 years ago

This PR adds a few new functions to Directory/File classes:

Changes to ITB_IO library also fixed an issue that caused Directory and File instances created via Directory:directory and Directory:file to be created with paths that have not been normalized.

Lemonymous commented 2 years ago

path returns the full path with a trailing slash, while relativize and relative_path return paths without the trailing slash (causing some current code to error)

Directory("scripts"):path() -- returns "C:/[..]/Into_The_Breach/scripts/" with a trailing slash
Directory("scripts"):relative_path() -- returns "scripts" without a trailing slash
Directory():relativize(Directory("scripts"):path()) -- returns "scripts" without a trailing slash
Lemonymous commented 2 years ago

Documentation for is_ancestor says path is an absolute path.

Using a relative path seems to work when dealing with paths building on the ITB directory, while not on the save data directory. I am not sure if this is intentional or not, but could this potentially cause confusion?

-- ITB directory
-- relative path
Directory("scripts"):is_ancestor("scripts/mod_loader")
-- returns true

-- absolute path
Directory("scripts"):is_ancestor(Directory("scripts/mod_loader"):path())
-- returns true

-- save data directory
-- relative path
Directory.savedata():directory("someProfile"):is_ancestor("someProfile")
-- returns false

-- absolute path
Directory.savedata():directory("someProfile"):is_ancestor(Directory.savedata():directory("someProfile"):path())
-- returns true
kartoFlane commented 2 years ago

Fixed is_ancestor accepting relative paths, now it should throw an error whenever a non-absolute path is passed. Also changed relative_path to have a native implementation in ITB_IO lib, that way we can more efficiently compute the correct path

Lemonymous commented 1 year ago

I am sorry that I dropped the ball on this PR.

I went through the following checks. Most of them passed. Some did not meet my expectation. I don't know if this is due to me using the functions incorrectly, or if they have not accounted for these cases.

local relative_path = "scripts/mod_loader" 
LOG(Directory("scripts"):is_ancestor(relative_path)) 
+ Expected: error
+ Result: error
local absolute_path = Directory("scripts/mod_loader"):path() 
LOG(Directory("scripts"):is_ancestor(absolute_path)) 
+ expected: true
+ result: true
local absolute_path = Directory():path() 
LOG(Directory("scripts"):is_ancestor(absolute_path)) 
+ expected: false
+ result: false
local relative_path = "Alpha" 
LOG(Directory.savedata():directory("Alpha"):is_ancestor(relative_path))
+ expected: error
+ result: error
local absolute_path = Directory.savedata():directory("Alpha"):path() 
LOG(Directory.savedata():directory("Alpha"):is_ancestor(absolute_path)) 
+ expected: true
+ result: true
local absolute_path = Directory.savedata():directory():path() 
LOG(Directory.savedata():directory("Alpha"):is_ancestor(absolute_path)) 
+ expected: false
+ result: false
LOG(Directory.savedata():directory(""))
+ expected: absolute path to save data directory
+ result: absolute path to save data directory
LOG(Directory.savedata():directory())
+ expected: absolute path to save data directory
+ result: absolute path to save data directory
LOG(Directory())
+ expected: absolute path to Into the Breach game directory
+ result: absolute path to Into the Breach game directory
LOG(Directory(""))
- expected: absolute path to Into the Breach game directory
- result: error "./scripts/mod_loader/bootstrap/itb_io.lua:321: Failed to create Directory instance for path "": called `Option::unwrap()` on a `None` value"
LOG(Directory("scripts"):path())
+ expected: "C:/.../Into the Breach/scripts/"
+ result: "C:/.../Into the Breach/scripts/"
LOG(Directory("scripts"):relative_path())
+ expected: "scripts/"
+ result: "scripts/"
LOG(Directory():relativize(Directory("scripts"):path()))
- expected: "scripts/"
- result: "scripts"
kartoFlane commented 1 year ago

Thanks for checking! I fixed both cases you've highlighted

Lemonymous commented 1 year ago

Thanks for the update! Both cases I highlighted are indeed fixed.

However, this case has a different result now

LOG(Directory("scripts"):relative_path())
- expected: "scripts/"
- result: "scripts//"

Maybe not a big deal, but do you want to fix it before merge?

kartoFlane commented 1 year ago

Drat, that's what you get for not writing tests. I'll push a fix in a moment

Lemonymous commented 1 year ago

Perfect! Thank you kartoFlane!

And sorry again for the long merge time.

kartoFlane commented 1 year ago

No worries, I haven't had the energy to work on programming-related stuff outside of work for the past six months, so I didn't mind this PR being put on hold :P