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 root path in file traversal #80

Closed
wants to merge 4 commits into from

Conversation

nuke-web3
Copy link

@nuke-web3 nuke-web3 commented Sep 28, 2023

Root path used if given, appending relative directory given in CLI if present.

Perhaps not the best handling of errors, as the root (if given in CLI or in config) will be used as the base path for the CLI path given directory and may be confusing if you don't realize the root is set in a config file, and the error informs you of some path you think should exist relative to your pwd path... 🤔

More verbose eprint provided 👍

This doesn't pickup the config from a provided root path, that I would expect it to though.

closes #78

src/file_traversal.rs Outdated Show resolved Hide resolved
@nuke-web3 nuke-web3 marked this pull request as draft September 28, 2023 19:57
Comment on lines 16 to 20
eprintln!(
"Relative path from privided root {:?} is malformed: {:?} --- {}",
root, relative_root, e
);
std::process::exit(1);
Copy link
Author

Choose a reason for hiding this comment

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

wooooo I have never seen this: the stdout printed is randomly truncated from right to left in parts, yielding only some of the message cutoff at the inserted variables or the text between.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved mostly using https://doc.rust-lang.org/core/result/enum.Result.html#method.unwrap_or_else

This can be randomly injected into other log lines being printed still. Like the header printout with mlc version

Comment on lines +93 to +102
directory = root_path
.join(&directory)
.canonicalize()
.unwrap_or_else(|e| {
eprintln!(
"Relative path from privided root {:?} is malformed: {:?} --- {}",
root_path, directory, e
);
std::process::exit(1);
});
Copy link
Author

Choose a reason for hiding this comment

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

Here I make the decision to canonicalize (and make this a full path) from the root path.


It does beg the question: what is the root path in conjunction with a directory path actually for? I think all I see root used as pointing to a config file location and place to walk flies within (kinda like this pattern: https://classic.yarnpkg.com/lang/en/docs/cli/#toc-cwd ). Walking otherwise would need to be set in the CLI directory argument... Why not instead remove the notion of a "root" with the provided directory being the source of walking files, and point optionally to a config file?

So in a config file you could set a relative path that the directory argument would default to if nothing was provided on the CLI.

This would perhaps be strange, as if the config was looking for a path that was not in the present working dir, that would fail, but at least in the present impl it would fail with an error showing the full path tried. Perhaps a warning that the config set this would be sufficient to help end users.
In a global config, for example, you would not want to set this argument at all, only on a per project basis.

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/becheran/mlc#usage

All links to the file system starting with a slash on linux or backslash on windows will use another virtual root dir. For example the link in a file [link](/dir/other/file.md) checked with the cli arg --root-dir /env/another/dir will let mlc check the existence of /env/another/dir/dir/other/file.md.

@nuke-web3
Copy link
Author

nuke-web3 commented Sep 28, 2023

Well I see this PR is flawed now, I didn't understand the use of root - closing this and #78

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.

.mlc.toml options are not used
1 participant