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

Add Directory:relativize and Directory:is_ancestor #181

Merged
merged 14 commits into from
Apr 15, 2023

Conversation

kartoFlane
Copy link
Member

@kartoFlane kartoFlane commented Nov 6, 2022

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

  • Directory:relativize - returns a path that can be used to move from the relative directory to the path passed as input to the function. Example:
    Directory("some/path"):relativize("some/path/test") -- returns "test"
    Directory("some/path/test"):relativize("some/path") -- returns ".."
  • Directory:is_ancestor - checks whether this directory is an ancestor to the specified absolute path
  • Directory:root - returns the root directory of this directory (either ITB directory or save data directory)
  • File:root - same as above
  • Directory:relative_path and File:relative_path have been changed to use native implementation in ITB_IO library, to reduce memory usage (previous implementation created a few intermediary strings, which needed to be cleaned up)

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.

@kartoFlane kartoFlane added the enhancement New feature or request label Nov 6, 2022
@Lemonymous
Copy link
Contributor

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

@Lemonymous
Copy link
Contributor

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 added a commit that referenced this pull request Nov 6, 2022
Fix relative_path working on paths containing magic characters

This is a stopgap measure to fix the current pre-release. #181 sees to fix it without needing magic regex
- Added Directory.root and File.root
- Added Directory.relative_path and File.relative_path
- Fix Directory.is_ancestor to throw an error for non-absolute paths
This way we return a trailing slash for Directory:relative_path,
without having to concatenate strings
@kartoFlane
Copy link
Member Author

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
Copy link
Contributor

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"

- Fix Directory.relativize not returning paths terminated with a slash
- Fix panic when creating a Directory from an empty string
@kartoFlane
Copy link
Member Author

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

Fix relativize returning trailing slash for non-directory paths
@Lemonymous
Copy link
Contributor

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
Copy link
Member Author

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

@Lemonymous
Copy link
Contributor

Perfect! Thank you kartoFlane!

And sorry again for the long merge time.

@Lemonymous Lemonymous merged commit 45a8afc into master Apr 15, 2023
@Lemonymous Lemonymous deleted the feature/itb-io-relativize branch April 15, 2023 23:17
@kartoFlane
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants