Naming conventions discussion

[SeppPenner]

Hi @Joelius300. What do you think: Should the chart dataset classes be named LineChartDataset or LineDataset like in DoughnutDataset? Can we discuss a convention here, maybe?

[SeppPenner]

Well, we should establish a naming convention for the other classes as well, e.g. DoughnutOptions or DoughnutChartOptions?

I would like to use the short way everywhere: We're using a charting library, so it should be clear that the configuration is for charts, actually 😄 --> DoughnutOptions, not DoughnutChartOptions.

But that's something you need to decide.

[SeppPenner]

And when we're talking about conventions already: Should we use . at the end of a <summary> or not?

/// <summary>
/// Creates a new instance of <see cref="LineChartData"/>
/// </summary>

or

/// <summary>
/// Creates a new instance of <see cref="LineChartData"/>.
/// </summary>

[SeppPenner]

Should usings be declared outside or inside the namespace? I'm asking because I normally use Resharper and the StyleCop extension and the default setting puts the namespaces inside (Second example).

E.g.

using System.Collections.Generic;

namespace ChartJs.Blazor.ChartJS.DoughnutChart
{
    //...
}

or

namespace ChartJs.Blazor.ChartJS.DoughnutChart
{
    using System.Collections.Generic;

    //...
}

[SeppPenner]

Another thing: Resharper suggests to use Gets or sets the title. as a comment for a public get and set property, e.g.:

/// <summary>
/// Gets or sets the percentage of the chart that is cut out of the middle.
/// </summary>
public int CutoutPercentage { get; set; } = 50;

Should we use this in the same way?

[SeppPenner]

For boolean values, I would recommend:

/// <summary>
/// Gets or sets a value indicating whether the title is displayed or not.
/// </summary>
public bool Display { get; set; }

[SeppPenner]

If you give me some advice / opinion here, I would like to help you out and adjust all classes. In the best case, this should take one day or so.

[MindSwipe]

I think using shorter names for the chart dataset classes is ok, as you said this is a chart library so the Chart part is basically implicit

I feel like adding a . to the end of the summary won't hurt, and is also standard in .NET (e.g Console.WriteLine) as well as the ASP.NET team also putting dots at the end, as seen here.

Personally, I like having my using statements outside of the namespace. There is a difference, not just style/ preference as discussed here on StackOverflow, but the .NET Core team have them outside so I vote we do that.

And lastly, I feel like starting the summary for properties off with "Gets or sets..." is a very succinct way of telling a developer what's going on. And is (again) also done by the .NET team (e.g Console.OutputEncoding)

[Joelius300]

Okay let's do this one by one.

By the way, I think it would be a good idea to expand the guidelines with the results of this discussion. Either rename and use the XML_Conventions or create a new file for this.

LineChartDataset vs LineChartDataset

I'm very open to the idea that we use LineDataset. This would mean using the short version everywhere (not only datasets).

Keep in mind that currently the default is the long version (LineChartDataset) for every dataset except a few that weren't named consistently.
For this reason we would need to rename all the others as well. We would either need one big change where we rename everything before we rework
anything else or we would need to change all these classes and files while reworking that particular chart.
In general I think consistency is really important so having some that use the short-version and others that use the long-version is not ideal.
That's why I would suggest updating all names at once (one commit per chart, all put on a separat branch for now).
Problem is, I don't have as much time for this project as I wish I did so it's hard for me to all do that in one go.

Do you have a different suggestion or should we try to do it like this?

Period / Full stop for summaries

That's one I'm still not sure myself.
I've gotten used to putting one whenever it's an actual sentence. If I'm not sure I usually put one.

Would you agree on this convention?

Usings inside or outside namespaces

Have you read this SO question about this?

We use usings outside of the namespace.
Not only is this the default for visual studio, it's how the project was up until now (and every other project of me).

Summaries on properties

The Gets or sets part is in general a good idea. However I have a few points to mention about this.

1. With this library, you will most likely never need to get the value of a property. You only define it and the js-interop tells chart.js what to do.
2. All these options are predefined by chart.js, we don't define how they're named and where they are placed (unless we want some custom serialization).
   Since chart.js has a description/summary for almost every property/possibility we have in our library (in their docs), I always used what was written there.
   This means copy what is written there, add <see/>-tags if they reference something which can be reasonably referenced within our library as well and maybe convert into a full sentence if it isn't already (only in fitting scenarios of course).

So I would say no, just use what's written in the chart.js docs (it's easier for us and people who are familiar with chart.js). If there is nothing in the chart.js-docs, definitely use Gets or sets.

EDIT:
I've just looked at part of the pull request you submitted and I saw that you just changed

The something and something

to

Gets or sets something and something

That is actually a quite nice solution as it doesn't loose the description by chart.js.
You can definitely use this. I don't know yet if I will change this for all of LineChart but it's definitely a great solution.

Summaries for boolean properties

I like the "Value indicating wether .. or not". However, as mention in the previous point, only fall back to that if there isn't a chart.js description.

MindSwipe

By the time I'm done with this @MindSwipe already mentioned some points which I totally agree on as you can see above. However the last point is, again, in general very good, but in this case not the right thing if there already is a description for basically the same option in the chart.js-docs.

Coding style of corefx

Take a look at the coding style of corefx.

In general, I will follow these but I have a few things to say before.

Braces (1)

The points about braces (always on newline) is very good.
A few things to mention though:

• I don't like if and using statements without braces.
  if with the conditional operation on the same line (js-like) is okay if there's only one short statement: if (done) Console.WriteLine("done");
• If we switch to c# 8 the using declarations will be here and those look quite nice so we'll be using them.

Indentation with spaces (2)

This really doesn't matter to me. You can use spaces or tabs, just always keep the amount of whitespace the same.

var (10)

I'm not a fan of var and prefer the explicit name 9 out of 10 times.
If there is a really long declaration or it's followed up by new xx I'm okay with it.

Consts PascalCasing (12)

Yes. However I'm in the habit of using "SOMETHING_LIKE_THIS". For the future, use PascalCasing.

Non-ASCII chars (15)

I've never done it like this but this would be a good idea since we're working with web-stuff where this kind of thing is even more sensitive.

Indent labels (16)

To be honest with you, I don't even know what this means.

[SeppPenner]

By the way, I think it would be a good idea to expand the guidelines with the results of this discussion. Either rename and use the XML_Conventions or create a new file for this.

Sure. It should be extended I guess.

Keep in mind that currently the default is the long version (LineChartDataset) for every dataset except a few that weren't named consistently.

If you don't mind, I can do that. I understand that it's a major change because it affects literally everything...

For the get and set stuff: I think, it's a good idea to keep the documentation from Chart.Js and add this Gets or sets just for convenience reasons as @MindSwipe pointed out.

I like the "Value indicating wether .. or not". However, as mention in the previous point, only fall back to that if there isn't a chart.js description.

That's okay for me as well.

I don't like if and using statements without braces.

Same here. I always add braces :D

I'm not a fan of var and prefer the explicit name 9 out of 10 times.
If there is a really long declaration or it's followed up by new xx I'm okay with it.

This means a change for me (I always use var), but it's okay.

Consts PascalCasing (12)

Okay, I used camel case with an upper case start (Or something like that, I don't really remember).

Non-ASCII chars (15)

I have never used non ASCII chars before. Who does even do this?? :D Ok, I know a lot of guys programming in their mother language... :D

[SeppPenner]

@Joelius300 Would you mind to summarize the discussion in the XML_Conventions (and rename the file probably) or should I do it?

I will also adjust my pull reuqest to meet the new conventions already, later.

[Joelius300]

I will update the conventions when I get time (so not now lol).

I have also reviewed your PR and requested some changes but overall it seems good.

Ps. camelCase with Upper case start is the literal definition of PascalCase :)
Pps. I think the non-ASCII chars mostly apply to string values. You don't use non-ASCII in variables or something like that.
I'm swiss so I need to watch out for ö, ä and ü (not in this library though) but yea, it's not something that will/should happen often.

[SeppPenner]

I will update the conventions when I get time (so not now lol).

I can maybe try to add a pull request there, too. (I will see how much time there is).

Ps. camelCase with Upper case start is the literal definition of PascalCase :)

Oh, then I was mistaking something, haha 👍

Pps. I think the non-ASCII chars mostly apply to string values. You don't use non-ASCII in variables or something like that.
I'm swiss so I need to watch out for ö, ä and ü (not in this library though) but yea, it's not something that will/should happen often.

Well, I'm from germany, so I have this 'problem' as well. I've never been using German terms in variable names though :D But sure, strings are a problem.

[MindSwipe]

If we switch to c# 8 the using declarations will be here and those look quite nice so we'll be using them.

Just a quick note: Using statements and using declarations actually differ in when they call Dispose on the object (but I assume you already know this)

Here's an Example:

public async Task UsingStatement()
{
    HttpResponseMessage response;
    using (var client = new HttpClient())
    {
        response = await client.GetAsync("https://hacker-news.firebaseio.com/v0/item/8863.json?print=pretty");
    } // client gets disposed here

    // Do stuff with the response
    if (response.IsSuccessStatusCode)
    {
        Console.WriteLine(await response.Content.ReadAsStringAsync());
    }
}

public async Task UsingDeclaration()
{
    using var client = new HttpClient();
    HttpResponseMessage response = await client.GetAsync("https://hacker-news.firebaseio.com/v0/item/8863.json?print=pretty");

    // Do stuff with the response
    if (response.IsSuccessStatusCode)
    {
        Console.WriteLine(await response.Content.ReadAsStringAsync());
    }
} // client gets disposed down here

Just keep this in mind when writing a piece of code that uses using

Also, aren't we already (by default) using C# 8.0 or am I misunderstanding this Microsoft docs page?