💡 Make validation method post-init & parameter-less

[atifaziz]

This PR demonstrates the idea discussed in my comment to #54.

One side benefit of a parameter-less Validate method is that the syntax trees for the declaration and invocation can be shared singletons.

One new downside I discovered while taking the idea for a spin is that argument names are lost and so you have to quote and sync them manually instead of being able to rely on nameof. 😢

If you like & agree with the idea, then feel free to merge the PR. Otherwise I consider it to be a demonstration/spike PR that can be closed if we conclude that this doesn't cut it.

[amis92]

In the end, I think this is the most reasonable approach to validation. After discussions in #54 I think it's the simplest of all possible approaches. It also lends itself quite well for future enhancements in other areas.

It does keep the cost of running validation on smallest changes, but the cost of avoiding it seems to be a little overblown (https://github.com/amis92/RecordGenerator/issues/54#issuecomment-519417538)

[atifaziz]

Just a nitpick to keep using nameof instead of literal string.

You mean this is just for this project, right? Any other project would have to hard-wire them?

I don't think we're completely out of the woods with this. Have a look at d0eff410d12e2b8797bc7d5e856194873f658f6a. The new value parameter for all With* members is named value and seeing an ArgumentNullException where its ParamName reads something else can be confusing. The demonstration in d0eff410d12e2b8797bc7d5e856194873f658f6a deliberately shows a breaking test.

In the end, I am undecided if it's a real issue or not because it's okay for methods to delegate to deeper methods that may validate and which may in turn have different parameter names.

[amis92]

In the end, I am undecided if it's a real issue or not because it's okay for methods to delegate to deeper methods that may validate and which may in turn have different parameter names.

That's exactly what I'd focus on. We're delegating checks deeper because of circumstances. The checks should use property name as that's what they're actually verifying, as well as because there are multiple scenarios that can throw this exception:

[Record]
public partial class MyRecord
{
  public string Name { get; }
  partial void Validate() {
    if (Name is null) throw new ArgumentNullException("???");
  }
}

[Fact]
public void Tests()
{
  var record = new MyRecord(null); // throws, parameter name is 'name'
  record.WithName(null); // throws, parameter name is 'value'
  new MyRecord.Builder { Name = null }.ToImmutable(); // throws, no actual parameter per se
}

[atifaziz]

So to sum up, let's use nameof(Property) for our tests. In user guidelines that's also what we can advertise. And then, in the end, it's up to a developer to write the exception code.

Have a look at 4076ebde486b317355b0a1e2b2e9f935a53a4050, where validation becomes this:

        partial void Validate() =>
            Validate(Name);

        static void Validate(string name)
        {
            if (name == null) throw new ArgumentNullException(nameof(name));
        }

So you delegate to another method to whom you pass only the fields needing validation. The additional helper validation has the effect of then also capturing the argument names for exception via nameof. What do you think? I feel it strikes a nice sweet spot and the test represents guidance too. 😄

[atifaziz]

I think rather than being prescriptive with a name like Validate, we should just call it OnConstructed or OnInitialized, which is just what it is. You can then do what you want in there, like validation. It also makes the following read better:

partial void OnInitialized() =>
    Validate(Name);

static void Validate(string name)
{
    if (name == null) throw new ArgumentNullException(nameof(name));
}

How do you feel about this?

[amis92]

I like the change of the name to OnConstructed.

Other than that, I'd call the rest as unimportant details. But for a guidance, I think it becomes over-engineered. If the validation is more complex, sure. But for the common case, I'd keep the exception throwing within the OnConstructed method.

As for the nameof usage, I think that using nameof(Property) is a good balance between throwing ArgumentNullException without argument name, and selecting arbitrary name (as is the case with the additional ValidateName(string)).

Property name is the thing that matters. Imagine seeing the stack trace. If the exception is thrown from within OnConstructed method with no parameters, it's a good guess that it was validating it's calling method, in this case constructor. And the constructor itself can be called by any With- method, ToImmutable, or some factory method user wrote.

---

For tests, let's use simple OnConstructed() with a body that throws ArgumentNullException(nameof(Property)).

For future guidance, do the same. Maybe suggest using custom ValidateProperty method to add a more meaningful stack trace entry.

[atifaziz]

I like the change of the name to OnConstructed.

Done with f0e212246824379babc725a5727fe166a373ad19.