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

Include the filename in the config file load errors #51713

Merged
merged 3 commits into from
Apr 26, 2021
Merged

Include the filename in the config file load errors #51713

merged 3 commits into from
Apr 26, 2021

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Apr 22, 2021 •
edited
Loading

  • Simplify source null check in FileConfigurationProvider
  • Add error messages to Resources/Strings.resx
  • Wrap load error with InvalidDataException and ensure it's captured/handled
  • Add and update unit tests to cover new logic
  • Added unit tests for:
    • InvalidDataException
    • FileNotFoundException
    • DirectoryNotFoundException
  • Adjusted existing unit tests

Fixes #36007

@ghost
Copy link

ghost commented Apr 22, 2021

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Simplify source null check in FileConfigurationProvider
  • Add error messages to Resources/Strings.resx
  • Wrap load error with InvalidDataException and ensure it's captured/handled
  • Move using Stream stream = OpenRead(file); within try / catch to ensure that exceptions thrown from OpenRead are captured
  • Add and update unit tests to cover new logic

Fixes #36007

Author: IEvangelist
Assignees: -
Labels:

area-Extensions-Configuration
Milestone: -

safern
safern reviewed Apr 22, 2021
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

It seems like this is a breaking change as FileNotFoundException nor DirectoryNotFoundException are base classes of InvalidDataException?

@IEvangelist
Fixed unit tests. Corrected resource error message, space/format. Rev…
f3d2c4e 
…ert OpenRead location outside try/catch. Added all possible exceptions to Load() comments.
@IEvangelist
Copy link
Member Author

IEvangelist commented Apr 23, 2021 •
edited
Loading

It seems like this is a breaking change as FileNotFoundException nor DirectoryNotFoundException are base classes of InvalidDataException?

@safern - corrected in f3d2c4e

@maryamariyan maryamariyan added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Apr 26, 2021
@IEvangelist IEvangelist merged commit bca13de into dotnet:main Apr 26, 2021
@IEvangelist IEvangelist deleted the file-path-on-load-error branch April 26, 2021 20:05
@karelz karelz modified the milestones: 5.0.x, 6.0.0 May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
@maryamariyan maryamariyan removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@eerhardt eerhardt Awaiting requested review from eerhardt

@ericstj ericstj Awaiting requested review from ericstj

@maryamariyan maryamariyan Awaiting requested review from maryamariyan

Assignees

@maryamariyan maryamariyan

Labels
area-Extensions-Configuration breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Include filename in load exception
4 participants
@IEvangelist @maryamariyan @safern @karelz