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

style: tidy up and expand comments in Activation #1268

Merged
merged 3 commits into from Feb 19, 2017

Conversation

Projects
None yet
4 participants
@olevett
Copy link
Member

olevett commented Feb 8, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
More comment tidying, in Activation this time

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

Other information:

@@ -54,6 +54,9 @@ public sealed class ViewModelActivator
/// <value>The deactivated.</value>
public IObservable<Unit> Deactivated { get { return deactivated; } }

/// <summary>

This comment has been minimized.

@olevett

olevett Feb 8, 2017

Author Member

Comments like this always feel...pointless, happy to remove it if wanted but thought since I'm here I may as well fix the compile time warning...

This comment has been minimized.

@ghuntley

ghuntley Feb 8, 2017

Member

Voted pointless, the return type is Unit

image

See above for another line that can be 🔥

This comment has been minimized.

@olevett

olevett Feb 8, 2017

Author Member

In terms of compiler warnings (can't remember exactly what you get for missing a returns...), would you prefer empty comments or nothing at all?

@@ -315,16 +343,29 @@ static IDisposable handleViewModelActivation(IViewFor view, IObservable<bool> ac

/// <summary>
/// This class implements View Activation for classes that explicitly describe
/// their activation via ICanActivate. This class is used by the framework.
/// their activation via <see cref="ICanActivate"/>. This class is used by the framework.
/// </summary>
public class CanActivateViewFetcher : IActivationForViewFetcher

This comment has been minimized.

@olevett

olevett Feb 8, 2017

Author Member

This feels like it should be in internal class, it's whole purpose in life is to be something used by the framework rather than end consumers, but that's definitely a breaking change/needs some discussion...

This comment has been minimized.

@ghuntley

ghuntley Feb 8, 2017

Member

Hmm, I don't know.

My perspective on breaking stuff has changed since starting work on video courseware; there has to be an incredibly good reason to do it as it wastes the time of consumers and my time if the courseware needs to be updated. @kentcb probably has similar feelings as he's going through this process right now with authoring a book.

I had a quick look at the GitHub for Desktop aka Visual Studio plugin source code and it has taken no references to this class. It's one of the larger open-source projects that uses ReactiveUI. https://github.com/github/VisualStudio/search?utf8=%E2%9C%93&q=CanActivateViewFetcher

This comment has been minimized.

@olevett

olevett Feb 8, 2017

Author Member

I'm wary of breaking things, but the pre-existing comment is along the lines of "used by the framework" and it's explicitly registered by ReactiveUI, so think its both fairly low risk and fairly low impact....

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage remained the same at 65.874% when pulling 69c8e98 on olevett:tidying-comments-activation into 51249d4 on reactiveui:develop.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 13, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 3300405 on olevett:tidying-comments-activation into 8e9963f on reactiveui:develop.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 19, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling 599a45a on olevett:tidying-comments-activation into eb7a36c on reactiveui:develop.

@kentcb kentcb added this to the vNext milestone Feb 19, 2017

@kentcb kentcb merged commit bf6e1a4 into reactiveui:develop Feb 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 65.938%
Details
@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 19, 2017

Thanks @olevett!

@olevett olevett deleted the olevett:tidying-comments-activation branch May 19, 2017

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