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

ud3/prettify: improve exception logging rules #118

Merged
merged 1 commit into from
Sep 1, 2017

Conversation

niezbop
Copy link
Member

@niezbop niezbop commented Sep 1, 2017

  • Restrict rule start to actual exceptions and prevent it from firing on other log with exceptions (for instance on updating/loading/building custom exception classes).

  • Adds alternative end condition to exception rule when the file that raised the exception is unknown (ThreadAbortException for example).

@niezbop niezbop requested a review from lacostej September 1, 2017 10:27
@@ -24,10 +24,10 @@
},
"exception": {
"active": true,
"start_pattern": "[eE]xception.*: (?<message>.*)\\n",
"end_pattern": "Filename: (?:[\\w/:]+/(?<file>\\w+\\.\\w+))? Line: (?<line>-?\\d+)",
"start_pattern": "[eE]xception\\w*: (?<message>.*)\\n",
Copy link
Member Author

Choose a reason for hiding this comment

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

This will only match Exception throwing now

"start_pattern": "[eE]xception.*: (?<message>.*)\\n",
"end_pattern": "Filename: (?:[\\w/:]+/(?<file>\\w+\\.\\w+))? Line: (?<line>-?\\d+)",
"start_pattern": "[eE]xception\\w*: (?<message>.*)\\n",
"end_pattern": "(?:Filename: (?:[\\w/:]+/(?<file>\\w+\\.\\w+))? Line: (?<line>-?\\d+)|(?<fileunknown>filename unknown)>:(?<lineunknown>-?\\d+))",
"start_message": false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Add alternative ending for exception thrown from unknown files

@lacostej
Copy link
Member

lacostej commented Sep 1, 2017

Could we add some tests, either unit or integration to start covering for those cases?

@niezbop
Copy link
Member Author

niezbop commented Sep 1, 2017

I'm not sure this is easily doable. Theses fixes are for edge cases that should occur quite rarely. If we want those to be covered we need an extensive log base for most (if not every) things doable with the Unity Editor and make sure and isolate the rule and challenge the log base, making sure that it only catches what was intended (how would you define that correctly?) and nothing else.

In this precise case, issues appeared when I was

  1. Creating .unitypackages for classes with custom exception classes
  2. Facing ThreadAbortException which seems to be unable to track a throwing file

@lacostej
Copy link
Member

lacostej commented Sep 1, 2017

If we don't have a set of test to go against, we will have a hard time refactoring the rules file without breaking it.

@niezbop
Copy link
Member Author

niezbop commented Sep 1, 2017

Agreed. I'm opening an issue for that then as I believe this should be done in a separate PR. Better start collecting logs!

@lacostej
Copy link
Member

lacostej commented Sep 1, 2017

Approved. Don't forget to make a better PR message for the changelog. Thanks!

@niezbop niezbop changed the title Exception rule fix Prettifier exception rule modification Sep 1, 2017
@lacostej lacostej changed the title Prettifier exception rule modification ud3/prettifier exception rule improvements Sep 1, 2017
@niezbop niezbop merged commit f564ec4 into DragonBox:master Sep 1, 2017
@lacostej lacostej changed the title ud3/prettifier exception rule improvements ud3/prettify exception rule improvements Sep 5, 2017
@lacostej lacostej changed the title ud3/prettify exception rule improvements ud3/prettify: exception rule improvements Sep 5, 2017
@lacostej lacostej changed the title ud3/prettify: exception rule improvements ud3/prettify: improve exception logging rules Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants