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

Do not print a error message in loadYamlSettings #172

Merged
merged 1 commit into from Jun 26, 2019

Conversation

@felixsch
Copy link
Contributor

commented Jun 25, 2019

While implementing the Data.Yaml.Config module I was stumbling over the this hardcoded putStrLn statement and I think it should be left to the user of the yaml library to decide how to handle exception and not print anything to stdout.

If I missed something and the putStrLn statement is worth keeping just ignore me and close the PR.

Thanks for you're awesome work!

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

Thanks for catching this, I definitely don't want a hard-coded putStrLn in here. However, this change will end up losing information on which file failed. Can I ask for a slightly different implementation: introduce a new exception type that includes both the FilePath in question and the underlying exception value and throws that instead of throwIO e. Basically, s/throwIO e/throwIO (LoadYamlSettingsException fp e). Does that make sense?

@felixsch felixsch force-pushed the felixsch:master branch from 275708d to 2d4ae4d Jun 26, 2019

@felixsch

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Good idea, I added the LoadSettingsException exception.

@snoyberg

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

Awesome, LGTM. One last request: can you add a minor version bump and a changelog entry?

@felixsch felixsch force-pushed the felixsch:master branch from 2d4ae4d to 041e01a Jun 26, 2019

@felixsch

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I added the changelog entry and bumped the version

@snoyberg snoyberg merged commit 041e01a into snoyberg:master Jun 26, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@snoyberg

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

Thanks! I made a slight tweak to make the version number 0.11.1.0 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.