Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Resolve unblocked TODOs and formatting changes to System.Net.Mail. #12750

Merged
merged 2 commits into from
Oct 18, 2016

Conversation

Priya91
Copy link
Contributor

@Priya91 Priya91 commented Oct 18, 2016

Address PR comments left by @CIPop

Also addresses TODO with issue #11747

cc @CIPop @stephentoub

[EventSource(Name = "Microsoft-System-Net-Mail", LocalizationResources = "FxResources.System.Net.Mail.SR")]
internal sealed class WebEventSource : EventSource
[EventSource(Name = "Microsoft-System-Net-Mail", Guid = "32663b9f-4d8d-44f8-9f29-f4efc12e6879", LocalizationResources = "FxResources.System.Net.Mail.SR")]
internal sealed class EmailEventSource : EventSource
Copy link
Member

Choose a reason for hiding this comment

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

EmailEventSource => MailEventSource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oki, will rename.

@@ -10,18 +10,18 @@

namespace System.Net
{
[EventSource(Name = "Microsoft-System-Net-Mail", LocalizationResources = "FxResources.System.Net.Mail.SR")]
internal sealed class WebEventSource : EventSource
[EventSource(Name = "Microsoft-System-Net-Mail", Guid = "32663b9f-4d8d-44f8-9f29-f4efc12e6879", LocalizationResources = "FxResources.System.Net.Mail.SR")]
Copy link
Member

Choose a reason for hiding this comment

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

In the past I believe @vancem's guidance has been to not explicitly specify a Guid, and to not worry about localizing resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely drop the Guid specification. If you do not the only way to turn this EventSource on is by Guid (you can't use its name).

    /// Overrides the default (calculated) Guid of an EventSource type. **Explicitly defining a GUID is discouraged**, 
    /// except when upgrading existing ETW providers to using event sources.

Localization of resource is only useful if you have an EXPLICT scenario where you are using this EventSource to send message to the Windows Event Log (the only logger that uses it). This is also a very specialized use case, so in general we recommend against, it but unlike the GUID issue it does not explicitly 'hurt' (it just adds unnecessary complexity).

Copy link
Contributor

Choose a reason for hiding this comment

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

Even we chose not to localize the resources, we place all strings in the resource file anyways. This is better than putting hard-coded strings in the *.CS files themselves.

So, I would prefer we keep this design as-is and leave strings in the resources file.

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 didn't add GUID initially for the reasons stated above, found an article about it on msdn blog. @CIPop left feedback on the merged PR #12416 to explicitly add a GUID and update windows debugging documentation, the discussion is here

cc @CIPop Are you ok with reverting this change or keeping the GUID?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that removing the Guid parameter does NOT mean that the EventSource does not have a GUID, it only means that the EventSource generates it from the name. In fact the GUID that it will have for the name Microsoft-System-Net-Mail will be
42c8027b-f048-58d2-537d-a4a9d5ee7038
However you can also use *Microsoft-System-Net-Mail in most tools instead (which is MUCH nicer).

Copy link
Contributor

Choose a reason for hiding this comment

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

However you can also use *Microsoft-System-Net-Mail in most tools

What about tools where you have to specify the GUID?

And we already have GUIDs for other event sources. See:
Documentation/debugging/windows-instructions.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove the GUID for now, @CIPop please file issue if you disagree. Thanks!

Copy link

Choose a reason for hiding this comment

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

@Priya91 @vancem

It's fine to remove the GUID but add it within both the source code as well as https://github.com/dotnet/corefx/blob/master/Documentation/debugging/windows-instructions.md#traces

However you can also use *Microsoft-System-Net-Mail in most tools instead (which is MUCH nicer).

I found this doesn't work logman.
@vancem please review https://github.com/dotnet/corefx/blob/master/Documentation/debugging/windows-instructions.md#traces and let me know if there is a way to use the *<name> format using existing Windows tools.

Copy link

Choose a reason for hiding this comment

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

Tracking with #12808.

@Priya91 Priya91 merged commit 2c446b2 into dotnet:master Oct 18, 2016
@Priya91 Priya91 deleted the prmail branch October 18, 2016 23:56
Copy link

@CIPop CIPop left a comment

Choose a reason for hiding this comment

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

Thanks @Priya91 ! Really appreciate seeing the nitpick fixed as well!

@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Resolve unblocked TODOs and formatting changes to System.Net.Mail.

Commit migrated from dotnet/corefx@2c446b2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants