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.

Sign up for GitHub

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

Jump to bottom

Return errors from ArcLoaderBuilder.build(). #9

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Return errors from ArcLoaderBuilder.build(). #9

merged 3 commits into from
Jun 15, 2020

Conversation

tmpfs
Copy link
Contributor

@tmpfs tmpfs commented Jun 14, 2020

Hope this is ok, cheers!

XAMPPRocky
XAMPPRocky requested changes Jun 14, 2020
View reviewed changes
Copy link
Owner

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! I've left a couple of nits, once those are addressed I would be happy to merge this in and put out a patch.

templates/src/error.rs Show resolved Hide resolved
templates/src/error.rs Outdated Show resolved Hide resolved
@tmpfs
Store path for shared bundle.
ba4de2e 
Passing the path back for resources will be convoluted as the
information is lost when fs::read_from_dir() is called.
@tmpfs
Copy link
Contributor Author

tmpfs commented Jun 15, 2020

Hi XAMPPRocky,

We can pass the path easily for the shared resource but it is lost for the locale resources and I think it would involve a fairly heavy refactor to include the path for the locale resources; whilst it is desirable I don't have the time to go through all the changes required to make this work.

No worries if you don't want to take the PR, cheers!

@XAMPPRocky
Copy link
Owner

Thanks for looking into this, that reasoning seems fine, to me. If you could remove the path variable so we only have one error variant I can merge this in and create a seperate issue for improving the error message.

@tmpfs
Copy link
Contributor Author

tmpfs commented Jun 15, 2020

Thanks. Agreed, I think it is better if assigning the path to the error is handled as a separate issue considering it will take some refactoring of the types to implement correctly.

Hopefully, the latest commit can be merged. Cheers 👍

@XAMPPRocky XAMPPRocky merged commit 4ae8745 into XAMPPRocky:master Jun 15, 2020
@XAMPPRocky
Copy link
Owner

This has now been published as 0.5.10

@github-actions github-actions bot mentioned this pull request Mar 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XAMPPRocky XAMPPRocky XAMPPRocky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmpfs @XAMPPRocky