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

Implement Project and project-relative resolving #141

Merged
merged 5 commits into from
Jul 20, 2020
Merged

Implement Project and project-relative resolving #141

merged 5 commits into from
Jul 20, 2020

Conversation

oberien
Copy link
Owner

@oberien oberien commented Jul 16, 2020

No description provided.

Copy link
Collaborator

@HeroicKatora HeroicKatora left a 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));
Copy link
Collaborator

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()),
Copy link
Collaborator

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.

@oberien
Copy link
Owner Author

oberien commented Jul 20, 2020

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.

@oberien oberien requested a review from HeroicKatora July 20, 2020 08:03
@oberien oberien merged commit 3fe482a into master Jul 20, 2020
@oberien oberien deleted the project branch July 20, 2020 08:19
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