-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
RegistryLayoutRenderer: Support for layouts, RegistryView (32, 64 bit) and all root key names (HKCU/HKLM etc) #925
Conversation
What do you mean with
I'm on mobile now, so I cannot easily see the code. But did you excluded the code for Silverlight? (see appveyor error) |
Silverlight has already been excluded in the old version and still is. So it should not be the problem. However the However some unit tests fail, but I am not quite sure, whether the unit tests themselves are correct. Why do you use |
We need to escape the
Just checked the code. The |
|
||
this.subKey = this.key.Substring(pos + 1).Replace('/', '\\'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this removal makes it non-backwards-compatible
This one needs comments.
We can make it also conditional with an property (default of course backwardscomp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I just moved it to ParseKey().
The reason, why the 3 tests fail, is, that I changed "string" to Layout for Key, Value and DefaultValue. Obviously |
We can fix it :) But not now. On holiday with limited Internet / pc. I will look at it this weekend. |
I'm looking at this now :) |
Looked at this:
if (pos >= 0)
{
hiveName = key.Substring(0, pos);
var subKey1 = key.Substring(pos + 1).Replace('/', '\\');
//remove duplicates
subKey1 = subKey1.Replace(@"\\", @"\");
subKey1 = subKey1.TrimStart('\\');
this.subKey = subKey1;
}
|
I will add some tests to the RegistryTests in master, because i'm curious how |
From MS documentation:
I do not see the problem. |
It's a false positive of Resharper :) Can you add this a comment in the code? |
Actually I had already done so, see line 142. |
I see the confusion. It about openSubKey. - my mistake. Posted the wrong code. https://msdn.microsoft.com/en-us/library/z9f66s0a(v=vs.110).aspx
So Reshaper was right :) |
I added some more tests in #937. if you like, you can merge them for locally testing: Git commands:
edit: well, merge is not necessary, only if you would like to debug locally at your site. |
TODO:
|
Hi @Niklas-Peter , The slash isssue has been resolved, see 118b8a5 The parse of the inner layout fixed is fixed in #975 Can you merge with master and fix the last issues? |
I will do that anytime, but currently I do not even have time to fix these small issues ... |
No problem. Any idea when you have time? The release of 4.2 is nearby, but we can postpone this feature to 4.3 |
If you like help this, just add me as a contributor and let me know. I like to have the feature implemented :) |
We need unit test for this. |
I added you as a collaborator to Niklas-Peter/NLog. Is that, what you intended? |
Yes, collaborator I meant :) |
- Layouts for Key, Value, DefaultValue - Added View - Root key names like "HKLM" are now allowed Note: I can not find any use for an not set "Value". If Value is null, you would always get the DefaultValue. Maybe you should make it a required parameter? Please check the code, especially your conventions for exception handling. ParseKey() and this.DefaultValue.Render() could throw an exception. I do not know, how you would like to handle it.
Full change:
|
@bhaeussermann / @page-not-found / @UgurAldanmaz can someone review this one? I like to include this in NLog 4.3 Thanks! |
</nlog>"); | ||
|
||
LogManager.GetLogger("d").Debug("zzz"); | ||
AssertDebugLastMessage("debug", "reg32"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these tests not use AssertLayoutRendererOutput()
like the other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there are old tests which also use this approach; can be changed to use AssertLayoutRendererOutput()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a difference. AssertLayoutRendererOutput
only renders the layout renderer, while these test cases really writes to the registry. So keep it like this
I checked the code. I cannot find any problems. Just two minor remarks on the tests. |
Thanks @bhaeussermann! |
Merge after successful builds |
RegistryLayoutRenderer: Support for layouts, RegistryView (32, 64 bit) and all root key names (HKCU/HKLM etc)
Please check the code, especially your conventions for exception handling. ParseKey() and this.DefaultValue.Render() could throw an exception. I do not know, how you would like to handle it.
Obviously Microsoft.Win32 is a problem. Maybe you can add appropriate pre compiler conditions to only activate it on supporting platforms.