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

Added Layout.CreateFromString with throwConfigExceptions parameter #3771

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 22, 2020

Trying to prevent cases where people globally enabled LogManager.ThrowExceptions to perform Layout-validation. See also:

elmahio/elmah.io.nlog@f26377e

And moved FuncLayoutRenderer-tests to SimpleLayoutParserTests.

Could also be convinced to rename it to TryCreateFromString. But then I'm not allowed to throw exception with reason why (and where) it failed.

@304NotModified
Copy link
Member

Could also be convinced to rename it to TryCreateFromString. But then I'm not allowed to throw exception with reason why (and where) it failed.

I prefer the TryCreateFromString (bool return, out parameter) + CreateFromString (which throws always an exception - in an error case ;).

It's really how MS is doing things and a "throw exception flag" is a known code smell AFAIK.

The only reason for not use the try-pattern is consistency which the current API (not the case I think).

So I vote for TryCreateFromString+CreateFromString :)

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 22, 2020
@snakefoot
Copy link
Contributor Author

Since CreateFromString is a new method, then we can change the contract so it throws on error. No problem with that. If that is what you mean?

But then I don't really need TryCreateFromString unless you suggest a contract where it can return false, but still return SimpleLayout-object?

@304NotModified
Copy link
Member

Since CreateFromString is a new method, then we can change the contract so it throws on error. No problem with that. If that is what you mean?

Yes indeed.

But then I don't really need TryCreateFromString unless you suggest a contract where it can return false, but still return SimpleLayout-object?

Not for this case indeed. But I like it as it's easier to use from the client. (no need to do a catch).

@snakefoot
Copy link
Contributor Author

Ok what method should you call to get the default behavior where SimpleLayout replaces unknown renderers with literals ?

@304NotModified
Copy link
Member

Ok what method should you call to get the default behavior where SimpleLayout replaces unknown renderers with literals ?

Ow forgot about that. (I wrote it myself years ago I think, lol).

Maybe we should keep it like this then :)

src/NLog/Layouts/Layout.cs Outdated Show resolved Hide resolved
src/NLog/Layouts/LayoutParser.cs Outdated Show resolved Hide resolved
@304NotModified 304NotModified changed the title Layout.CreateFromString with throwConfigExceptions-parameter Added Layout.CreateFromString with throwConfigExceptions parameter Jan 22, 2020
…uncLayoutRenderer-tests to SimpleLayoutParserTests
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Thanks!

@snakefoot snakefoot added this to the 4.7 milestone Jan 22, 2020
@sonarcloud
Copy link

sonarcloud bot commented Jan 22, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

83.3% 83.3% Coverage
0.0% 0.0% Duplication

@repo-ranger repo-ranger bot merged commit 45f7b03 into NLog:master Jan 22, 2020
@snakefoot
Copy link
Contributor Author

Updated wiki: https://github.com/NLog/NLog/wiki/Configure-from-code

@snakefoot snakefoot added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Jan 23, 2020
@snakefoot snakefoot deleted the FromLayoutMethod branch April 4, 2020 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) enhancement Improvement on existing feature needs documentation on wiki size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants