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

Make path-based inputs generic of AsRef<Path> #73

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

jakelishman
Copy link
Member

This simplifies calling code from outside the library in various places; various components no longer need to be compelled or recollected into the exact typing that the input requires, but instead can be used in anything that can be treated as a path slice.


When calling from Qiskit side, it's quite inconvenient to re-collect everything I get natively as str or whatever from Python space into an owning PathBuf, where it's already interpretable via reference into a Path-like (OsString is a superset of String for use in file-system access, and OsString: AsRef<Path> since Path is give-or-take a wrapper around &OsStr (which in turn is to OsString as &str is to String)).

This simplifies calling code from outside the library in various places;
various components no longer need to be compelled or recollected into
the exact typing that the input requires, but instead can be used in
anything that can be treated as a path slice.
@jakelishman
Copy link
Member Author

This isn't the absolutely most efficient use of the generics - likely if we tightened up the access modifiers a little bit there'd more a more natural place at which we canonicalise the typing / require ownership / whatever, but immediately this is just making the typing of the interface a little more Rust-y.

The requirement to specify the type of the None variant is a bit unfortunate, but ties in to what I was saying elsewhere about getting rid of the environment search within the parser library; imo that value shouldn't be an Option at all, it should only be a slice and the environment var should be read by the user-facing front-end, not the parser internals.

Comment on lines +132 to 144
if let Some(paths) = search_path_list {
for path in paths {
if let Some(full_path) = try_path(path.as_ref()) {
return full_path;
}
}
} else if let Some(paths) = search_paths() {
for path in paths {
if let Some(full_path) = try_path(path.as_ref()) {
return full_path;
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't ideal with collecting a generic from the user input but also hard-coding access to the environment, but it's well wrapped, and this way doesn't involve collecting a Vec from the environment if nothing's given.

@jlapeyre
Copy link
Collaborator

jlapeyre commented Jan 22, 2024

Thanks. I thought the interface was too rigid. But I opted to get it done as quickly as possible.

The requirement to specify the type of the None variant is a bit unfortunate

On the other hand, it makes the call site more readable and understandable. Seeing things like f(v, true, None, false) is kind of sad. (Maybe 21st century editors make this less of a problem, IDK)

getting rid of the environment search within the parser library; imo that value shouldn't be an Option at all, it should only be a slice and the environment var should be read by the user-facing front-end, not the parser internals.

As I said, I was concentrating on getting file inclusion and error reporting working. Using the env variable was quick. But the more I think about it, the more I agree it doesn't belong in the bowels of the library. This is a bit junky, and it is propagated through calls:

pub fn parse_source_file(
file_path: &PathBuf,
search_path_list: Option<&Vec<PathBuf>>,

I imagine in the future, more information will need to be passed to parse_source_file, say regarding dialects. The search path could be hidden nicely in a struct. Even so, it seems more of a font-end concern.

I am not opposed to removing the environment variable stuff, depending on how it plays out. I don't see off the top of my head how a specification of a search path at a higher level can be propagated to the search for included files. (I mean without actually propagating it, which is what we want to remove!)

@jakelishman
Copy link
Member Author

At some point, I imagine the answer is that we'll want to wrap up a larger amount of configuration into some sort of Config struct with convenient builder methods, and then both things like the semdemo binary and Qiskit will construct that struct and it'll just be a single object to thread through.

@jlapeyre jlapeyre merged commit 4d1bcb5 into Qiskit:main Jan 22, 2024
7 checks passed
@jakelishman jakelishman deleted the fix-path-typing branch January 22, 2024 19:57
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