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

StackTraceLayoutRenderer with Raw format should display source FileName #2075

Merged
merged 1 commit into from
Apr 25, 2017
Merged

StackTraceLayoutRenderer with Raw format should display source FileName #2075

merged 1 commit into from
Apr 25, 2017

Conversation

snakefoot
Copy link
Contributor

Attempt to fix #2074

@snakefoot snakefoot force-pushed the StackTraceLayoutRendererWithSource branch 2 times, most recently from 5398185 to d05cc33 Compare April 24, 2017 19:55
@codecov-io
Copy link

codecov-io commented Apr 24, 2017 •
edited
Loading

Codecov Report

Merging #2075 into master will decrease coverage by <1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #2075    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         289     289            
  Lines       19908   19908            
  Branches     2353    2354     +1     
=======================================
- Hits        16204   16190    -14     
- Misses       3105    3118    +13     
- Partials      599     600     +1

@snakefoot snakefoot force-pushed the StackTraceLayoutRendererWithSource branch 3 times, most recently from e586d50 to 804a36e Compare April 24, 2017 20:21
304NotModified
304NotModified reviewed Apr 24, 2017
View reviewed changes
tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs Outdated

RenderMe("I am:");

AssertDebugLastMessageContains("debug", "RenderStackTrace_RawWithSource at offset ");
Copy link
Member

Choose a reason for hiding this comment

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

not sure if this a useful test now. It can be still file:line:column <filename unknown>:0:0 ?

Copy link
Contributor Author

@snakefoot snakefoot Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Well it is combination of the 3 asserts, that all verifies the same last logged message. See the original RenderStackTrace_Raw, which does the same.

Copy link
Member

Choose a reason for hiding this comment

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

yeah but should we not test that line number >0 ?

Need a regex I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it is enough for me that it was able to capture the filename (StackTraceRendererTests.cs). Not wanting to dive into the StackTrace-formating abyss.

Copy link
Member

Choose a reason for hiding this comment

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

something like this?

string pattern = @"<(?<filename>.*?)>:(?<line>\d+):(?<col>\d+)";

https://regex101.com/r/JJ1tUk/1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not committing any regex funkyness. Just close this PR, and create you own :)

Copy link
Member

@304NotModified 304NotModified Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

am I allowed to push on this branch? (dunno where I can see that, it's a github option)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Allow edits from maintainers" is checked, so I guess you can knock yourself wild :)

src/NLog/LayoutRenderers/StackTraceFormat.cs Outdated
/// <summary>
/// Raw format (multiline with source info - as returned by StackFrame.ToString() method).
/// </summary>
RawWithSource,
Copy link
Member

Choose a reason for hiding this comment

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

I'm doubting if we should patch raw instead of a new option (I know i proposed it earlier ;))

Printing file:line:column <filename unknown>:0:0 sounds useless to me and the default is flat.

What do you think?

Copy link
Contributor Author

@snakefoot snakefoot Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not using this LayoutRenderer, so fixing the existing Raw-format is fine with me. Just give a shout, and I will remove RawWithSource and fix Raw.

Copy link
Member

Choose a reason for hiding this comment

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

I think fix "raw" is more clear :)

So remove and we need to document the change in Nlog 4.4.7.

@304NotModified
Copy link
Member

304NotModified commented Apr 24, 2017 •
edited
Loading

Thanks! I was thinking to create this, but you we're faster :)

@304NotModified 304NotModified added bug Bug report / Bug fix enhancement Improvement on existing feature feature and removed enhancement Improvement on existing feature labels Apr 24, 2017
@304NotModified 304NotModified added this to the 4.4.7 milestone Apr 24, 2017
@304NotModified 304NotModified mentioned this pull request Apr 24, 2017
Closed
@snakefoot snakefoot force-pushed the StackTraceLayoutRendererWithSource branch 2 times, most recently from d6e7ed1 to ba09168 Compare April 24, 2017 21:23
@snakefoot snakefoot changed the title StackTraceLayoutRenderer and StackTraceFormat.RawWithSource StackTraceLayoutRenderer with StackTraceFormat.Raw should display source FileName Apr 24, 2017
@snakefoot snakefoot changed the title StackTraceLayoutRenderer with StackTraceFormat.Raw should display source FileName StackTraceLayoutRenderer with Raw format should display source FileName Apr 24, 2017
@snakefoot snakefoot force-pushed the StackTraceLayoutRendererWithSource branch from ba09168 to 42fc955 Compare April 24, 2017 21:30
@snakefoot snakefoot force-pushed the StackTraceLayoutRendererWithSource branch from 42fc955 to 999e4eb Compare April 24, 2017 21:46
304NotModified
304NotModified reviewed Apr 24, 2017
View reviewed changes
tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
AssertDebugLastMessageContains("debug", "<filename unknown>");

#if !MONO
AssertDebugLastMessageContains("debug", "StackTraceRendererTests.cs");
Copy link
Member

Choose a reason for hiding this comment

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

👍

@304NotModified 304NotModified merged commit f2ece32 into NLog:master Apr 25, 2017
@304NotModified
Copy link
Member

updated docs for this: https://github.com/NLog/NLog/wiki/Stack-Trace-Layout-Renderer

@snakefoot snakefoot deleted the StackTraceLayoutRendererWithSource branch October 10, 2017 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified left review comments

Assignees
No one assigned
Labels
bug Bug report / Bug fix feature
Projects
None yet
Milestone
4.4.7
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@snakefoot @codecov-io @304NotModified