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

Path and MetaData objects + wezterm.<functions> #4493

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

Danielkonge
Copy link
Contributor

@Danielkonge Danielkonge commented Oct 26, 2023

This pull request is a combination of #4330 and #4343, but I have now added the Path and MetaData objects discussed in the matrix chat.

This ended up being a quite large pull request, and I have a few different things of note to discuss before this is ready to be merged.

  • Should we include wezterm.home_path. I haven't made any other wezterm.<name>_dir commands, but I do think it makes sense to do for wezterm.home_dir. (The current PR includes wezterm.home_path.)
  • Does it make sense to allow multiple sorts in wezterm.read_dir? The current code allows you to sort the directories multiple times since I thought that might be useful for sorting stuff with a lot of overlaps in one of your sorting criteria. I.e., if you source by the length of the names of files, and you have a lot of files with names of 4 or 5 characters, then you can additionally sort by some other criteria like time since you last accessed the file first. I haven't found much use for this though, and one sort only could simplify the code.
  • Should wezterm.read_dir return strings instead of Path objects? It used to return strings, but for now I have changed it to return Path objects. Because of a workaround mentioned in the notes at the end, I don't think this should cause any problems.
  • I am not sure if there is a solution for this, but can it really be true that there is no async Rust function like try_exists? The other functions of this type use smol::fs to make it async, but smol::fs doesn't seem to have a try_exists.
  • The current __concat meta method for the Path object treats the paths as strings and does string concatenation (since I think that is more intuitive). Would it be better to do a Rust join of the paths instead?
  • Should the Path objects include non-async (or async via metadata/symlink_metadata) versions of is_dir, is_file and is_symlink? For now I didn't include these, since I think that it is fine to get these through path:metadata():is_<dir or file> or path:symlink_metadata():is_<symlink>().
  • Consider adding file_prefix when it is stable in Rust.
  • I currently pass around a lot of mlua::MultiValues in the Path versions of the string functions, since I think it gives nicer errors, because you get the errors of string.<func_name> that you expect. Is it better to add more details in the Rust code? (Then we could write more descriptive errors, but I think it complicates the code and I don't like these errors more anyways.)
  • Does the current selection of wezterm.<func_name> functions I have included in this PR make sense? Should I include something else? Or leave something out?

Notes:

  • To get around most of the problems with people treating Path objects as strings, I have implemented all the string functions where the first argument is a string (not formatting string or other type), since these are the ones people expect to be able to use as methods. So if path is a Path object, you can do something like path:gsub(...). The way I implement these is by converting the Path object to a string and then calling the string method with the given arguments. I don't try to change it back to a Path object at the end, since if you use the string method, you presumably want a string.
  • Although all the e2e::sftp test errors look like they might be related to this pull request since they also work with filesystem stuff, they don't have anything to do with this pull request. Those tests also fail on the current main branch.

Please let me know if you have any comments or questions in general about this pull request.

@wez
Copy link
Owner

wez commented Dec 2, 2023

(I'm holding off looking at this in earnest until the table PR is merged, because this one is layered on top of that and I prefer not to have to mentally filter it out here)

@Danielkonge
Copy link
Contributor Author

(I'm holding off looking at this in earnest until the table PR is merged, because this one is layered on top of that and I prefer not to have to mentally filter it out here)

That's fine. I plan to do another cleanup of this too taking care of some similar problems as in the other PR.

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.

2 participants