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?

[SeppPenner]

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

I guess not:

The feature "using-declarations" is currently in preview and is not supported. To use preview functions, use the language version "Preview".

[Joelius300]

@MindSwipe We're not targeting .net core 3 at the moment. We're targeting .net standard 2.0, that's why it doesn't default to c# 8 but to c# 7.3.

Do we want to change this? It probably wouldn't have any disadvantages..

EXCEPT the nonnullable-reference stuff.. It probably wouldn't be too hard to add a few question marks to a few properties but it would have to be done. We could also just stay away from this feature for now. Or we stay on c# 7.3.

[SeppPenner]

If you ask me, I would already go with C#8. I mean, we will need to adjust this if we want to move to .NetCore 3.0 sometimes either way...

[MindSwipe]

I feel like making the move to C# 8.0 wouldn't bring any harm, as nullable reference types are optional and we would manually need to opt in to by adding <NullableContextOptions>enable</NullableContextOptions> to the .csproj file.

Also, even if we do opt in to nullable reference types, worst that happens is Visual Studio throws a warning our way but it would still compile and work

[SeppPenner]

From #50:

We have actually always used Ticks instead of Tick. We should continue with that because it's a config for a set of elements and the property is also always plural. Furthermore e.g. PointLabel seems to be a single element (from the name) but actually it's a config and applies to multiple point-labels. Other such cases include: Ticks, GridLines, Scales (only LineChart), PointLabels, .. I'll add this to the conventions (#22) if I don't forget.

@Joelius300: Note to you :)

[SeppPenner]

We should really update this file now. Otherwise, we will forget something... https://github.com/Joelius300/ChartJSBlazor/blob/master/XML_Conventions.md (And rename it probably.)

[Joelius300]

Yes this is the most important thing after we're done with 0.10.3. We need do complete this file before any other charts are reworked. I can work on it from time to time and will open a PR so any missing stuff can be thrown at me :)

[SeppPenner]

• Do you prefer the short version or using this.something by the way? (Same for base.something, of course) E.g. in a constructor:

public A(B b)
{
    this._b = b;
}

or

public A(B b)
{
    _b = b;
}

• Should private variables be prefixed with a _ or not?
• We should add a license header once everything is stable enough to not get changed all day:

// --------------------------------------------------------------------------------------------------------------------
// <copyright file="Class.cs" company="Joel L., ChartJsBlazor contributors">
//   MIT License

//   Copyright (c) 2019 Joel L.

//   Permission is hereby granted, free of charge, to any person obtaining a copy
//   of this software and associated documentation files (the "Software"), to deal
//   in the Software without restriction, including without limitation the rights
//   to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
//   copies of the Software, and to permit persons to whom the Software is
//   furnished to do so, subject to the following conditions:

//   The above copyright notice and this permission notice shall be included in all
//   copies or substantial portions of the Software.

//   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
//   IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
//   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
//   AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
//   LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
//   OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
//   SOFTWARE.


//   The underlying library from https://github.com/mariusmuntean/ChartJs.Blazor
//   is also available under the MIT license. The LICENSE.md file is missing but it's written in
//   https://github.com/mariusmuntean/ChartJs.Blazor/blob/master/src/ChartJs.Blazor/ChartJs.Blazor.csproj.  
//   If https://github.com/mariusmuntean (the author of the underlying library) sees this and has a problem 
//   with how I use the library or with how I mention the MIT license of his library, please contact me.
// </copyright>
// <summary>
//   Class summary.
// </summary>
// --------------------------------------------------------------------------------------------------------------------

[Joelius300]

Only use this where necessary. This is adapted from point 4 in the corefx coding-style. base is also never used unless necessary.

Yes, private variables are prefixed with _. In our library we don't have many of those (because it's almost all just models) but when there are we should prefix them with _. Coplies with point 3 of the corefx coding-style.
I guess I didn't do that from the start (samples don't use _). That's not very important (consistency is more important) but yes, it would be better if we used _ for the private fields there.

Are you sure you want those license headers? I always found them to be a bit obnoxious with little to no advantage.. Can you present me some good arguments to add those?

[SeppPenner]

Only use this where necessary. This is adapted from point 4 in the corefx coding-style. base is also never used unless necessary.

Okay. I guess, I will need another Resharper settings file for that (I'm using the style from my company and therefore I see a lot of issues all the time 😄).

I will check the private fields as well and see what I can do :)

Are you sure you want those license headers? I always found them to be a bit obnoxious with little to no advantage. Can you present me some good arguments to add those?

Well, I didn't use them before as well because they bloat up the files. It was just a suggestion from the code style my company uses. If you don't want to add it, I'm ok as well. We have the option to only include a reference to the license file instead of copying the whole license, as well. But again, just a suggestion.

[SeppPenner]

Another thing here: Do we want this code style for empty braces:

(I saw that you (@Joelius300) did it like this in the line chart rework).

[Joelius300]

I did this because I didn't want to add two basically empty lines. As far as I'm concerned, the other common way to do it would be using two lines like so:

public MyClass() : base()
{
}

I don't really know which one I'd choose but since this library has been using { } much more than with two lines, we should continue with that for consistency sake. If we really want we can search and replace all of them at a later time :)