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

Include start / end in exceptions thrown by type inspector #624

Conversation

jairbubbles
Copy link
Contributor

@jairbubbles jairbubbles commented Jul 15, 2021

  • Make sure to include Start / End in exception thrown by type inspector
  • Use message from the serialization exception directly

@jairbubbles jairbubbles force-pushed the include-start-end-in-exceptions-thrown-by-type-in branch from 3f3c232 to 573515f Compare July 15, 2021 19:59
Copy link
Owner

@aaubry aaubry left a comment

Choose a reason for hiding this comment

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

I think including the location of the error is a useful improvement, however I don't think that changing ITypeInspector to take a scalar is a good idea for the following reasons:

  • That interface should not be coupled to YAML structure. It is an abstraction over the .NET type system. In principle it could be used in contexts that have nothing to do with YAML.
  • This is a breaking change that will force anyone who implements that interface to change their code. ITypeInspector is a frequently used extension point, so the impact would be high.

Instead I would suggest to catch the exception(s) and wrap them. In principle, this would also be a breaking change, although I believe that the impact would be quite low as I don't expect code to rely on these exceptions specifically.

What do you think?

@jairbubbles
Copy link
Contributor Author

@aaubry As it was not massively used in the solution I thought that maybe it was an internal interface.I tried my luck! ☺

By the way there is this nice analyzer to detect breaking changes in the public API:
https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md

Please note that I'm currently using YamlDotNet to create a linter for a .yml file and appart from the issue fixed here, it would have been nice to receive multiple serialization exceptions. For example you have a missing property at the beginning of the file and a syntax error at the end.

We could accumulate the serialization errors and wrap them in a new exception type like MultipleYamlException. What do you think @aaubry?

@jairbubbles jairbubbles force-pushed the include-start-end-in-exceptions-thrown-by-type-in branch from 573515f to 87118e7 Compare July 16, 2021 03:48
@jairbubbles jairbubbles requested a review from aaubry July 16, 2021 03:52
@jairbubbles
Copy link
Contributor Author

Hi @aaubry, I changed the code accordingly to your remarks, what do you think about it?

@aaubry
Copy link
Owner

aaubry commented Aug 4, 2021

Hi @aaubry, I changed the code accordingly to your remarks, what do you think about it?

Sorry, I missed your review request. I'll review it asap.

@aaubry
Copy link
Owner

aaubry commented Aug 6, 2021

Thanks and sorry for the delay!

@aaubry aaubry merged commit 5f91b4f into aaubry:master Aug 6, 2021
@jairbubbles
Copy link
Contributor Author

Thx! Let me know when you release a version with the fix.

@jairbubbles
Copy link
Contributor Author

Hi @aaubry any plans to release a version including this fix?

@aaubry
Copy link
Owner

aaubry commented Jul 23, 2022

This feature has been released in version 12.0.0.

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.

None yet

2 participants