Skip to content
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

Confirm/update private access modifier in RC topics+samples #11010

Closed
guardrex opened this issue Feb 19, 2019 · 14 comments
Closed

Confirm/update private access modifier in RC topics+samples #11010

guardrex opened this issue Feb 19, 2019 · 14 comments

Comments

@guardrex
Copy link
Collaborator

guardrex commented Feb 19, 2019

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.

@danroth27
Copy link
Member

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

@guardrex
Copy link
Collaborator Author

Don't tell Steve that! He won't think that he's part of the engineering team. 😄 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.

@guardrex
Copy link
Collaborator Author

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
    {
        ...
    }
}

@NTaylorMullen
Copy link
Contributor

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

@{
    WeatherForecast[] forecasts;
}

@functions {
    WeatherForecast[] forecasts;
}

Being explicit with access modifiers makes it much more clear:

@{
    WeatherForecast[] forecasts;
}

@functions {
    private WeatherForecast[] forecasts;
}

@guardrex
Copy link
Collaborator Author

guardrex commented Feb 20, 2019

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 👍 ... 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 👁️ it. This awaits your orders, too. I'll open an issue and work it if you're Go! for throttle up. 🚀

@Eilon
Copy link
Member

Eilon commented Feb 20, 2019

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.

@guardrex
Copy link
Collaborator Author

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.

@Eilon
Copy link
Member

Eilon commented Feb 20, 2019

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

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.

@guardrex
Copy link
Collaborator Author

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.

@SteveSandersonMS
Copy link
Member

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

@scottsauber
Copy link
Contributor

scottsauber commented Aug 12, 2019

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).

@scottsauber
Copy link
Contributor

scottsauber commented Aug 12, 2019

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. 😄

/cc @guardrex

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 12, 2019

@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. #13523 Thanks anyway for the offer to help ... much appreciated. 👍

@scottsauber
Copy link
Contributor

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

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

No branches or pull requests

6 participants