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

FYI — directories / clio_extended #10

Closed
max-sixty opened this issue Jun 9, 2023 · 6 comments
Closed

FYI — directories / clio_extended #10

max-sixty opened this issue Jun 9, 2023 · 6 comments

Comments

@max-sixty
Copy link
Contributor

max-sixty commented Jun 9, 2023

Just an FYI that over at PRQL we wanted our CLI to also be able take a directory, and so added some logic to clio in our crate.

We're very open to feedback on whether we're doing reasonable things!

Here's the code: https://github.com/prql/prql/blob/c1150818f4497c89164ba9ec9468459acb8a191c/prql-compiler/prqlc/src/cli.rs#L409

Feel free to close as soon as you've seen this. Thanks.

@aj-bagwell
Copy link
Owner

This is a neat idea. I have released version 0.3 which adds a struct for tracking a path (which may be stdin/stdout) and was the perfect place to add a files method to list files in a directory.

So you should now be able to do something like:

    pub fn read_to_tree(path: ClioPath) -> anyhow::Result<SourceTree<String>> {
        let mut sources = HashMap::new();
        for file in path.files(has_extension("prql"))? {
            let mut file_contents = String::new();
            let path = file.path().to_owned();
            file.open()?.read_to_string(&mut file_contents)?;

            sources.insert(path, file_contents);
        }

        Ok(SourceTree::new(sources))
    }

(well ignoring the relative path name tidying up you do)

Does that work? I contemplated making it return a Vec<Input> but worried about it running out of file handles.

@max-sixty
Copy link
Contributor Author

ClioPath looks great! I think that will work well. Thanks!

I'm CCing @aljazerzen who wrote most of the code on our end if he has any thoughts.

files looks fine — I'm not even sure it's needed; can folks just pass a ClioPath to WalkDir? It would almost be passable exactly (i.e. without a &*path) if it implemented AsRef<Path>, though not sure whether there are other reasons not to do that...

@aljazerzen
Copy link
Contributor

Yup, ClioPath does it: PRQL/prql#2847

@max-sixty
Copy link
Contributor Author

Wonderful! I'll close...

@aj-bagwell
Copy link
Owner

Glad it was useful, of you have any other changes you want to make pull request are always welcome.

I didn't implement AsRef<Path> as passing a ClioPath to File::open would then work but defy the entire point of clio which is the - handling.

Passing it to walkdir would have similar issues, whereas the files method returns a Vec containing the original ClioPath if it is not a directory.

To be honest I should probably remove the Deref impl and pick some proper semantics for all Path methods and reimplemented properly.

Is stdin a file? Does it have metadata? Does it exist? What about URLs?

@max-sixty
Copy link
Contributor Author

Ah, that makes a lot of sense. Thanks for the explanation...

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

No branches or pull requests

3 participants