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

update names per naming guidelines. #1012

Merged
merged 7 commits into from Sep 8, 2016

Conversation

Projects
None yet
6 participants
@BillWagner
Member

BillWagner commented Sep 7, 2016

Fixes #1011

The section on "events" incorrectly recommended naming events with the "On" prefix. Events should be the verb or verb phrase, and the handlers should have the "On" prefix.

@BillWagner

This comment has been minimized.

Member

BillWagner commented Sep 7, 2016

Hmmm. OPS build didn't happen. closing and reopening.

@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 7, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 7, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

@@ -126,15 +126,15 @@ that the event objects can only be accessed in safe ways. The only
operations available on a field-like event are add handler:
```cs
EventHandler<FileFoundArgs> handler = (sender, eventArgs) =>
EventHandler<FileFoundArgs> OnFoundFile = (sender, eventArgs) =>

This comment has been minimized.

@svick

svick Sep 7, 2016

Collaborator

This is a local variable, so it should use camelCase, not PascalCase.

@@ -221,7 +221,7 @@ Let's update the subscriber so that it requests a cancellation once
it finds the first executable:
```cs
EventHandler<FileFoundArgs> handler = (sender, eventArgs) =>
EventHandler<FileFoundArgs> OnFoundFile = (sender, eventArgs) =>

This comment has been minimized.

@svick

svick Sep 7, 2016

Collaborator

This is a local variable too, so it should also use camelCase.

@@ -270,7 +270,7 @@ need extra code in those handlers in this project, but this shows how
you would create them.
```cs
internal event EventHandler<SearchDirectoryArgs> OnChangeDirectory
internal event EventHandler<SearchDirectoryArgs> ChangeDirectory

This comment has been minimized.

@svick

svick Sep 7, 2016

Collaborator

Wouldn't DirectoryChanged be a better name? It would also be more consistent with FoundFile and the guideline in events-overview to use past tense.

Also, why is changeDirectory just below a private event? Wouldn't a field be enough? (The text below also mentions that it's a field, when it's not currently.)

@@ -93,15 +93,18 @@ when there are no subscribers to that event.
You subscribe to an event by using the `+=` operator:
```cs
EventHandler<FileListArgs> handler = (sender, eventArgs) =>
EventHandler<FileListArgs> OnProgress = (sender, eventArgs) =>

This comment has been minimized.

@svick

svick Sep 7, 2016

Collaborator

One more local variable that should use camelCase.

@@ -48,7 +48,7 @@ properly sort the elements. LINQ queries must be supplied with delegates
in order to determine what elements to return. Both used a design built
with delegates.
Consider the `OnProgress` event handler. It reports progress on a task.
Consider the `Progress` event handler. It reports progress on a task.

This comment has been minimized.

@svick

svick Sep 7, 2016

Collaborator

Should this be "the Progress event"? I think this paragraph talks about the event itself, not about some specific event handler subscribed to it.

Respond to feedback
@svick provides the usual very thorough review.
@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 7, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 7, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

```
The type of the event (`EventHandler<FileListArgs>` in this example) must be a
delegate type. There are a number of conventions that you should follow
when declaring an event. Typically, the event delegate type has a void return.
Prefix event declarations with 'On'.
The remainder of the name is a verb. Use past tense (as in this example) when
Event declarations should be a verb, or verb phrase.

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

nit: or a verb phrase

present tense indicates that the event supports cancellation. For example,
an `OnClosing` event may include an argument that would indicate if the close
an `Closing` event may include an argument that would indicate if the close

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

an Closing -> a Closing

@@ -62,13 +62,13 @@ internal SearchDirectoryArgs(string dir, int totalDirs, int completedDirs)
}
public class FileSearcher
{
public event EventHandler<FileFoundArgs> OnFoundFile;
internal event EventHandler<SearchDirectoryArgs> OnChangeDirectory
public event EventHandler<FileFoundArgs> FoundFile;

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

I think the noun should come first and then the verb. So it would be FileFound event and DirectoryChanged event.
Some examples: ControlRemoved, SystemColorsChanged, UserPreferenceChanging, etc.

@terrajobst might know all the rules since he drives the framework design discussions.

This comment has been minimized.

@BillWagner

BillWagner Sep 7, 2016

Member

I looked, and didn't find any statement either way in the Framework Design Guidelines, but if there is a convention, I'll follow it.

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

That is just my observation on actual API names. I haven't looked at the guidelines.

This comment has been minimized.

@BillWagner

BillWagner Sep 7, 2016

Member

Even if it's not written, but a de facto guideline, I'll make the change. Waiting for @terrajobst to weigh in.

the event reports something that has happened. Use a present tense verb (for
example, `OnClosing`) to report something that is about to happen. Often, using
example, `Closing`) to report something that is about to happen. Often, using

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

Closing would be present participle no? And we also have some that are simple present that don't have cancellation such as Authenticate. Any recommendation on when to use one vs. the other?

This comment has been minimized.

@BillWagner

BillWagner Sep 7, 2016

Member

On the present participle: I'd prefer not going into that much grammatical detail for developers. Would you agree?
On the guidance: I'll do a bit of research and add a paragraph.

This comment has been minimized.

@mairaw

mairaw Sep 7, 2016

Contributor

Sounds good.

@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 7, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

@BillWagner

This comment has been minimized.

Member

BillWagner commented Sep 8, 2016

There's one remaining open feedback item. I'll propose this:

Proposal:

Change event names that are verb phrases from VerbNoun (ChangedDirectory) to NounVerb (DirectoryChanged).

Remain silent on whether this is a standard or not.

Can I get upvotes @mairaw and @terrajobst ? Once that happens, I'll make that change and merge.

@terrajobst

This comment has been minimized.

Member

terrajobst commented Sep 8, 2016

Change event names that are verb phrases from VerbNoun (ChangedDirectory) to NounVerb (DirectoryChanged).

👍 That's what the framework does.

@mairaw

This comment has been minimized.

Contributor

mairaw commented Sep 8, 2016

LGTM!

@qinezh

This comment has been minimized.

Contributor

qinezh commented Sep 8, 2016

Open Publishing Build Service: The pull request content has been published and here are some sample preview links:

@BillWagner BillWagner merged commit 59cb2f6 into dotnet:master Sep 8, 2016

3 checks passed

OpenPublishing.Build Open publishing build succeeded.
Details
OrcaBot [.NET Core - Nix] Samples built successfully on Ubuntu (x64).
Details
OrcaBot [.NET Core - Win] Samples built successfully on Windows (x64).
Details

@BillWagner BillWagner deleted the BillWagner:update-events-article branch Sep 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment