-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement Project and project-relative resolving #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolution and canonicalization seem fine but we should really, really switch to an error for main instead of panicking. Tell me if you're fine with doing it as part of this PR or a separate one.
let file = match cfgfile_folder.as_ref() { | ||
Some(folder) => { | ||
let file = folder.join("heradoc.toml"); | ||
let content = fs::read_to_string(&file).expect(&format!("error reading config file at {:?}",file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A panic on user input?
// 2. next to the input file | ||
// 3. somewhere in the parent folders of the input file | ||
let cfgfile_folder = match args.configfile.as_ref() { | ||
Some(file) => Some(file.parent().expect("configfile passed as argument should point to a file and not the root-folder").to_owned()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have enough of these panic-unwraps above as well. Oh well, it might be time for a proper error with variants.
Thanks for the review! Currently, we always panic on user input regarding command line arguments. I created #138 to track moving to proper diagnostics during argument usage and config parsing / migration. However, that depends on #137, which will be quite a huge refactor. I don't want to block this PR on all of that mountain of work, especially because currently we just panic everywhere before we call into the generator. |
No description provided.