SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
890 stars 46 forks source link

Generate instance after Util.GenerateValidation(item) inside the Generators #415

Closed ThomasSkyldahl closed 1 year ago

ThomasSkyldahl commented 1 year ago

Describe the feature

Currently the generator generates the instance returned from the From method before the validation has been run, could the instance be moved after the validation to avoid invalid stances being created and save a possible allucation?

Current code:

        /// <summary>
        /// Builds an instance from the provided underlying type.
        /// </summary>
        /// <param name=""value"">The underlying type.</param>
        /// <returns>An instance of this type.</returns>
        public static {className} From({itemUnderlyingType} value)
        {{
            {GenerateNullCheckIfNeeded(item)}

            {Util.GenerateNormalizeInputMethodIfNeeded(item)}

            {className} instance = new {className}(value);

            {Util.GenerateValidation(item)}

            return instance;
        }}

Suggested code:

        /// <summary>
        /// Builds an instance from the provided underlying type.
        /// </summary>
        /// <param name=""value"">The underlying type.</param>
        /// <returns>An instance of this type.</returns>
        public static {className} From({itemUnderlyingType} value)
        {{
            {GenerateNullCheckIfNeeded(item)}

            {Util.GenerateNormalizeInputMethodIfNeeded(item)}

            {Util.GenerateValidation(item)}

            {className} instance = new {className}(value);

            return instance;
        }}

From my own testing this breaks almost all the snapshot tests of course but none of the Vogen.Tests break.

ThomasSkyldahl commented 1 year ago

@SteveDunn I would be happy to do a pull request, if you agree its okay to break the snapshot tests, is there a easy way to regenerate the snapshots?

SteveDunn commented 1 year ago

That looks like a great idea - thank you @ThomasSkyldahl !

I've updated the readme on how to rebaseline the snaphsot files: https://github.com/SteveDunn/Vogen#ive-done-a-change-that-means-the-snapshot-tests-are-expectedly-failing-in-the-build---what-do-i-do

SteveDunn commented 1 year ago

Hi @ThomasSkyldahl - how's things going? I updated the readme again with some instructions that simpler and less likely that the 'overwrite' switch will be left on.

https://github.com/stevedunn/vogen#ive-done-a-change-that-means-the-snapshot-tests-are-expectedly-failing-in-the-build---what-do-i-do

The command line is just: Build.ps1 -resetSnapshots $true (and sit back for 40 minutes or so)

Please do let me know if I can help with this one.

ThomasSkyldahl commented 1 year ago

I have been on vacation over the weekend 😎 but I'm home again so I will have a pull request ready some time this week 🙂

SteveDunn commented 1 year ago

Excellent - thank you. Hope you had a great vacation!

ThomasSkyldahl commented 1 year ago

@SteveDunn yep it was great! Another thing there are a few tests that are not stable across time zones I'm currently at (GMT+2) so I have done a fix for it that set the local time zone for the duration of the test, but setting it requires a bit of reflection and if the tests are running with multithreading I'm not sure its stable but it does fix the issue,

The problematic tests are around the dapper tests with the in-memory SQLite for DateTime and DateTimeOffset. I think I will have the pull request ready tonight, then you can look into it and see if you want it solved another way.

ThomasSkyldahl commented 1 year ago

FYI: the mentioned issue with the tests is the same as described here: https://github.com/SteveDunn/Vogen/issues/430

SteveDunn commented 1 year ago

Thanks @ThomasSkyldahl - yes, I'm currently looking at the tests relating to time zone issues

ThomasSkyldahl commented 1 year ago

@SteveDunn I added the pull request #436 for the above + fixes for timezone and culture issues