Aspnetcore.docs: Confirm/update private access modifier in RC topics+samples

Created on 19 Feb 2019  路  14Comments  路  Source: dotnet/AspNetCore.Docs

There's at least one ... and probably a few more lurking in the code examples+sample apps. This happened because it was the case in the old Blazor engineering repo, examples, samples, etc.

@danroth27 to confirm that we need to always provide the private access modifier where currently in a handful of spots its defaulting to private.

Notes to self: Check Razor syntax topic. Confirm attribute location on same line or different line.

Note to self: Engineering PR with this.

Blazor P1

All 14 comments

@guardrex When faced with important decisions like this I do as @Eilon would do :smiley:. I believe the current practice of the ASP.NET engineering team is to always put the private access modifier.

_Don't tell Steve that!_ He won't think that he's part of the engineering team. :smile: lol

Here's an example from the template (and the class WeatherForecast just under it) ...

https://github.com/aspnet/AspNetCore/blob/master/src/Components/Blazor/Templates/src/content/BlazorStandalone-CSharp/Pages/FetchData.cshtml#L38

Most of the docs are now leaning toward private when private. I'll react to your/Steve's/their guidance when we know. I'll use this issue to either use private everywhere or update to use it nowhere.

ping @NTaylorMullen ... can you help? TL;DR ... are we using the private access modifier in Razor @functions blocks?

@functions {
    private WeatherForecast[] forecasts;

    protected override async Task OnInitAsync()
    {
        forecasts = await Http.GetJsonAsync<WeatherForecast[]>("sample-data/weather.json");
    }

    private class WeatherForecast
    {
        ...
    }
}

... or as the templates have it ...

@functions {
    WeatherForecast[] forecasts;

    protected override async Task OnInitAsync()
    {
        forecasts = await Http.GetJsonAsync<WeatherForecast[]>("sample-data/weather.json");
    }

    class WeatherForecast
    {
        ...
    }
}

are we using the private access modifier in Razor @functions blocks?

Yes, should try and avoid using implicit access modifiers in the functions block. ASP.NET engineering guidelines aside, my biggest reason is that using implicit accessor modifiers can unintentionally give the impression that code in the @functions block is synonymous with code in a @{...} block.

i.e. these mean two very different things

```C#
@{
WeatherForecast[] forecasts;
}

@functions {
WeatherForecast[] forecasts;
}

Being explicit with access modifiers makes it much more clear:
```C#
@{
    WeatherForecast[] forecasts;
}

@functions {
    private WeatherForecast[] forecasts;
}

Thanks @NTaylorMullen.

@danroth27 If you/Steve provide final approval, I'll use this issue to track the work for a sweep in the RC/Blazor docs. Most of our stuff is :+1: ... just a couple of spots to touch.

If we proceed with the access modifier, the RC/Blazor templates require the update. Let me know if you want me to open an issue/send a PR for those.

We should look broadly, too, at aspnet/Docs. There aren't many examples of @functions blocks in this doc set; however, best to :eye: it. This awaits your orders, too. I'll open an issue and work it if you're _Go!_ for throttle up. :rocket:

BTW how we write the product code certainly doesn't need to match what templates or docs do. They have two completely different audiences and different purposes.

Template code is often "simpler" whereas for the framework code itself we set some more rigid guidelines that help us maintain this codebase, sometimes at the cost of verbosity.

My current guidance from APEX is to follow engineering guidelines for docs and doc samples. I won't make a move on this until I receive guidance from @danroth27 and @SteveSandersonMS.

BTW for example the default console app template in VS has this Program.cs file:

```c#
using System;

namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine("Hello World!");
}
}
}
```

No modifiers on anything. That's totally fine for many cases in template code. I would imagine for many cases in docs that's fine as well.

Ultimately the question is which is better in each case for the consumer to understand. I think in many docs the modifiers are often noise, unless they carry a significant meaning, such as explaining that public methods on MVC controllers are publicly accessible, and non-public ones are not.

carry a significant meaning ... modifiers are often noise

@NTaylorMullen has a reason in this scenario for them, but @SteveSandersonMS doesn't embrace the modifier AFAIK, so he might be leaning that way.

We're in no giant rush. We have time to iron these bits out. I wasn't going to work this until after our pass on Razor Components Topic Reviews #10717 for Preview 2 is complete.

I'm totally happy with the inclusion of private (or other default) modifiers if that's what broader conventions specify.

I'm a little late to the bikeshed party (and not sure my opinion matters), but I'm updating my VS Code snippets for Blazor, because it is a few previews behind (my bad). I wanted to match the conventions on the Docs, and I was pretty surprised to see the private access modifier on all the docs for [Parameter]s. For instance on this doc there was the following code in the code block:

[Parameter]
private string Title { get; set; }

This seems weird to me. I feel like the access modifier convention for [Parameter]s should be public or protected for a couple reasons:

  1. If you decide to inherit from a ComponentBase and move all your @code block to the new ComponentBase, then you can't access any of your properties anymore from your .razor file and have to switch them all to protected, public, etc.

  2. The mental model for this is weird with private for me as a C# dev. Say I have a ChildComponent.razor file:

<h1>@Title</h1>

@code {
    [Parameter]
    private string Title { get; set; }
}

It seems odd that I can do this from a ParentComponent.razor file:

<ChildComponent Title="Some title I can change" />

Because my mental model in my head says Title is a private auto property - therefore I shouldn't be able to change it from another class which is what ParentComponent is.

Those are the two main reasons why I feel like they should be public or even protected (although number 2 is still a bit weird for protected, but less so).

Side note - I feel strong enough about this that I'd happily take the time to send the docs PR to switch all these if there's agreement on this. :smile:

/cc @guardrex

@scottsauber Engineering reversed their earlier approach and guidance. They're already reversed to public. The changes are on the staging branch for the Pre8 release. https://github.com/aspnet/AspNetCore.Docs/pull/13523 Thanks anyway for the offer to help ... much appreciated. :+1:

@guardrex - Whoops - sorry I missed that thread... my bad.... nothing to see here. :smile:

Was this page helpful?
0 / 5 - 0 ratings