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

Add default template support #21

Merged
merged 26 commits into from
Jul 12, 2021
Merged

Add default template support #21

merged 26 commits into from
Jul 12, 2021

Conversation

ostev
Copy link
Contributor

@ostev ostev commented Jul 11, 2021

Closes #16.

I pulled in the home crate to handle getting the home directory on Windows, which appears to require interacting with Win32 APIs.

ostev and others added 7 commits July 10, 2021 17:03
This is used for `bonnie --init`. It fetches the file at `$BONNIE_TEMPLATE_PATH`, falling back to `~/.bonnie/template.toml` if necessary.  The `home` crate was introduced to handle the finding of the user's home directory on Windows, which is surprisingly complicated and **can fail**.
Using the `-e` or `--edit-template` flag, the user can now open the template file using the editor in the `EDITOR` environment variable or, on Windows, `start`.
It is now `BONNIE_TEMPLATE` to match `BONNIE_CONFIG`.

This breaks with the previous implementation.
Changed `BONNIE_TEMPLATE_PATH` to `BONNIE_TEMPLATE`.
Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

This looks really great! Just a few minor things. The only main issues are that I'd like you to create string errors at the site of the error and then propagate those up to main, rather than returning any unparsed errors. Also, if you could move the -e code out of main.rs and into it's own file for testing, that'd be great.

I'm happy if this particular PR doesn't include testing, I can write tests for the features afterward.

src/help.rs Outdated Show resolved Hide resolved
src/help.rs Outdated Show resolved Hide resolved
src/init.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
ostev and others added 14 commits July 12, 2021 10:53
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Copying a file doesn't take that long.
It added quotes around a string.
Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Two tiny things left, and then we can push!

src/bin/main.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Minor changes.

src/template.rs Outdated Show resolved Hide resolved
src/bin/main.rs Show resolved Hide resolved
ostev and others added 2 commits July 12, 2021 12:57
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Co-authored-by: arctic_hen7 <arctic_hen7@pm.me>
Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

This looks good!

@arctic-hen7
Copy link
Owner

Just that match statement and then we can merge!

@arctic-hen7 arctic-hen7 self-requested a review July 12, 2021 21:01
Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Let's merge!

Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Tests have failed, do you mind running cargo fmt briefly? You can make sure they run with the bonnie check script in the root.

Copy link
Owner

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

All good now!

@arctic-hen7 arctic-hen7 merged commit 7c3eec6 into arctic-hen7:main Jul 12, 2021
@arctic-hen7
Copy link
Owner

Thank you very much for your contribution @ostev! It's greatly appreciated!

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.

Global template support
2 participants