Remove sorting by name from 'Record' class type

[SaebAmini]

I don't see a good reason for sorting constructor parameters by name. Following the natural flow of properties as they are defined in the specs seems like a better approach since related properties are usually defined close to each other (or at least they should be).

[RicoSuter]

Some tests are failing

[SaebAmini]

Yeah, sorry should have been not lazy and have properly cloned and changed stuff locally rather than edit in the browser 😄 I'll fix that now.

[SaebAmini]

Should be all good.

[RicoSuter]

/cc @ili is this ok even if it's a breaking change? To me it makes sense to keep the order (alphabetical doesnt make sense in most cases)

[ili]

Hi all!

Yes, it is heavy breaking change (in my case, for ex. hundreds code lines should be changed), and it is dangerous one.

Sorting is done from my own experience: you see, most POCO's fields are int, bool, string and other primitive types. Also they are changing from version to version, and there are no garantee that developer would add new field at the end of class. In some moment it would be added in the middle, and while actualizing clien's code you can miss this point and compiler can miss this too because of primitive types (may be all of your fields are int, for example).

Also reflection does not garantee members order (!!!).

All this points make using "native" order dangerous. Certyainly alphabetic sorting does not save in 100% cases, but it helps.

First time when we inventied records there were no sorting, but after week or two of development and changes we'v realized that we need to introduce some fixed order for records, that's why alphabetic sorting was done.

If you want to use native order some parameter sould be invented, to set the order with alphabetiacal as default.

[RicoSuter]

I think the right way to use these record classes is with named ctor params, to avoid this problems, e.g.

new Person(firstName: "Foo", lastName: "Bar")

this way a change in the params only breaks if it doesnt have a default value.

[ili]

This is the solution from one side. From another we do have hundreds lines of code with calling to POCO constructors :) May be not only we. This is heavy breaking change...

[SaebAmini]

Definitely agree that using named arguments are the way to go with potentially large API DTOs, that's what we're doing as well. Otherwise the code would be just too brittle and painful to read.

...it is dangerous one. Sorting is done from my own experience: you see, most POCO's fields are int, bool, string and other primitive types. Also they are changing from version to version, and there are no garantee that developer would add new field at the end of class.

Why do you think a guarantee of devs adding fields at the end is a good thing? and why is them not doing so is "dangerous"?

Consider this example:

public class Person
{
    public Person(string firstName, string lastName, DateTime birthday, double weight, double height)
    {
        FirstName = firstName;
        LastName = lastName;
        Birthday = birthday;
        Weight = weight;
        Height = height;
    }

    public string FirstName { get; }
    public string LastName { get; }
    public DateTime Birthday { get; }
    public double Weight { get; }
    public double Height { get; }
}

Let's say after some time the developer wants to also add a MiddleName. Adding it at the end would be nonsensical:

Person(string firstName, string lastName, DateTime birthday, double weight, double height, string middleName)

Sorting alphabetically, which is what's happening now, would be just plain ugly:

Person(DateTime birthday, string firstName, double height, string lastName, string middleName, double weight)

Fields and properties should be added to a class where they make sense, grouped with other semantically related fields and properties.

In some moment it would be added in the middle, and while actualizing clien's code you can miss this point and compiler can miss this too because of primitive types (may be all of your fields are int, for example).

Can you provide an example for this? and explain how sorting alphabetically solves that problem?

Also reflection does not garantee members order (!!!).

Not sure how this is relevant. Here we'd just be following the order in the provided OpenAPI specs document. There is no reflection on this end. The idea is that you should respect the provider of that document and their ordering.

First time when we inventied records there were no sorting, but after week or two of development and changes we'v realized that we need to introduce some fixed order for records, that's why alphabetic sorting was done.

I'm keen to know more about the actual problems you were dealing with and why alphabetic sorting was the best way to deal with them. So far the only point I agree with is that this is definitely a breaking change.

[ili]

Why do you think a guarantee of devs adding fields at the end is a good thing? and why is them not doing so is "dangerous"?

You would need to check it manyally, there is no restrictions, and developer can add new field in the beginning, for ex. Also some refactoring tools (resharper, for example) can change property's order during clean up. And reflection does not garantee members order. Also POCOs can be inherited one from other. All this points can effect. Certainly, using named parameters solves the problem, but this change is still breaking.

Can you provide an example for this? and explain how sorting alphabetically solves that problem?

That does not solve the problem. But minimizes risks of changes. The simpliest sample is many to many relation, when se do have a record:

Relation(int thisId, int otherId)

Let's imagine what will happen if somebody will change the order and nobody will beat his hands. Compillation will succeed, and tests will pass (we do have all ID's in DB, constraint does not fail).

Here we'd just be following the order in the provided OpenAPI specs document. There is no reflection on this end.

Open API spec is generated using reflection, for ex.

I'm keen to know more about the actual problems you were dealing with and why alphabetic sorting was the best way to deal with them.

It is not the best solution, it is ensurance from occasional code changes. Main point is risk minification.

[SaebAmini]

And reflection does not garantee members order... Open API spec is generated using reflection, for ex.

There is no reflection here. You have an OpenAPI specification document, and you're generating a client from it. As far as you're concerned, the owner of that document is specifying the order. How they're generating the document is pretty much irrelevant on this side. Although, as it happens, even the current implementation maintains the original order.

That does not solve the problem. But minimizes risks of changes... Let's imagine what will happen if somebody will change the order and nobody will beat his hands.

Your sample shows how this PR's change would be a risk for existing code, which I understand and agree with. It's just a shame that this change was ever introduced in the first place. The sample fails to demonstrate how sorting alphabetically was ever a good solution, to begin with.

It is not the best solution, it is ensurance from occasional code changes. Main point is risk minification.

Still waiting for an explanation of how it was even a mediocre solution. It's just relying on the pure coincidence of where a new field would be positioned alphabetically.

I'll leave the decision to you @RicoSuter.

[RicoSuter]

I agree with @SaebAmini here, the problem is that it is a breaking change. In any case, for record types you NEED to specifiy all parameter names to ensure no breaking changes after generation and in this case the order does not matter anyway. The PR will be revisited when planning NJS v11. For now you can replace the "Class.Constructor.Record.liquid" with your own sorting (TemplateDirectory setting => see wiki)

[RicoSuter]

Patiently waiting for native C# support of record types.. maybe we should consider already implementing the expected api (see c# draft)

[manfred-brands]

I also don't like the sorting of constructor parameters, e.g. endTime coming before startTime.
However this change can be easily done in a backward compatible way by adding a SortConstructorParameters property to CSharpGeneratorSettings with a default values of true to make this a backward compatible change.
This means @SaebAmini and myself can set that parameter to false and @ili doesn't need to do anything.

Related, I would like to optional create record types instead of class. Again to ensure backward compatibility this has to be done either using a settings property or introducing a CSharpClassType.CSharpRecord.

With this we don't have to wait for Version 11.
@RicoSuter If you agree, I can augment/replace this PR.

[RicoSuter]

@manfred-brands fine for me...
do we even need SortConstructorParameters now that we have "real" record types? :-)

Again to ensure backward compatibility this has to be done either using a settings property or introducing a CSharpClassType.CSharpRecord.

Here i wonder what's better, current Type is "Record" right?

[manfred-brands]

do we even need SortConstructorParameters now that we have "real" record types? :-)

We still need to create a constructor, at least if Nullable is enabled, You cannot declare a non-nullable property like:
public string Name { get; init; } without a constructor as it will result in a compile warning: CS8618: Non-nullable property 'Name' must contain a non-null value when exiting constructor.
The required keyword mooted for C#10 is still in the proposal state.

Personally, I prefer:

return new TimeRange(startTime, endTime, name);

over

return new TimeRange
{
    StartTime = startTime,
    EndTime = endTime,
    Name = name,
};

Here i wonder what's better, current Type is "Record" right?

The comment for CSharpClassStyle.Record says POCO and is still needed for those that haven't moved to C#9.

But if we generate init properties, you can say: updated = old with { Name = newName }
However init requires .NET5 or the IsExternalInit nuget package.
So to allow targeting earlier versions, I will make the init part optional as well.

Created #1399

[RicoSuter]

Closed in favor of
#1399