I believe this can be rolled back, as isValueNull is sufficient.
The current model before this change is:

      "_enum" : [ "affinity-data", "dealroom", null ],
      "allowableValues" : {
        "enumVars" : [ {
          "name" : "AffinityData",
          "isString" : false,
          "value" : "'affinity-data'"
        }, {
          "name" : "Dealroom",
          "isString" : false,
          "value" : "'dealroom'"
        }, {
          "name" : "Null",
          "isString" : false,
          "value" : "'null'"
        } ],
        "values" : [ "affinity-data", "dealroom", null ]
      },

and after:

      "_enum" : [ "affinity-data", "dealroom" ],
      "allowableValues" : {
        "enumVars" : [ {
          "name" : "AffinityData",
          "isString" : false,
          "isNullValue": false,
          "value" : "'affinity-data'"
        }, {
          "name" : "Dealroom",
          "isString" : false,
          "isNullValue": false,
          "value" : "'dealroom'"
        }, {
          "name" : "Null",
          "isString" : false,
          "isNullValue": true,
          "value" : "'null'"
        } ],
        "values" : [ "affinity-data", "dealroom", null ]
      },

you can see there is an oddity in allowableValues, where the value is already serialized to 'null', however, #19027 didn't change this I believe, as it only touched modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptClientCodegen.java not the DefaultCodegen.

The 'null' value is set here:

maybe the surrounding loop:

needs the guard as well and we just remove the value from the possible enum values altogether? WDYT?

@wing328 whats the core guidance here?

I am just trying to guard the second part:

diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
index f27e4a400e4..9215daee503 100644
--- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
+++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
@@ -6630,6 +6630,9 @@ public class DefaultCodegen implements CodegenConfig {
                 : 0;
 
         for (Object value : values) {
+            if(value == null) {
+                continue;
+            }
             Map<String, Object> enumVar = new HashMap<>();
             String enumName = truncateIdx == 0
                     ? String.valueOf(value)

and see how it will affect the samples. My guess is that it might be the better solution, as we don't introduce another template variable, keep the model more sane and it will fix all future generators as well.

yes that would be nice, and it would work for all generators without modifying the templates.

Okay, I will make the change later.

Just OOI: is there a better way to crate fixtures for a very specific part of the generation? It feels like quite the overkill to generate a whole scaffold with 10+ files just to assert one tiny part of it.
For the model change in this PR I could write a unit test, but generally, is there a way to create a partial scaffold in any way?

just pushed 01209d5 with the changes; seems cleaner (have a look at the diff)?

looks good, thanks!

Just OOI: is there a better way to crate fixtures for a very specific part of the generation? It feels like quite the overkill to generate a whole scaffold with 10+ files just to assert one tiny part of it.
For the model change in this PR I could write a unit test, but generally, is there a way to create a partial scaffold in any way?

yeah unit tests are the more compact/preferred option for these specific aspects

yeah unit tests are the more compact/preferred option for these specific aspects

I can change this PR to use unit tests if/when @wing328 approves and we merge it.

this test seems to be identical to modules/openapi-generator/src/test/java/org/openapitools/codegen/php/PhpModelTest.java - it seems maybe these files / tests were duplicated at some point. Might be worth moving this up onto a higher level and remove the duplication?

this is the line of the fixture that shows the issue

nullability (e.g. the possible value null) is declared here already.

Before the change in this PR, there is an additional line

Null = 'null'

for new TS sample folder, do we need to add it to the Circle CI script to ensure the output compiles without issues?

https://github.com/OpenAPITools/openapi-generator/blob/master/CI/circle_parallel.sh#L76

I guess, yes, would it make sense to determine this:

automatically?

added bbe6d34 for now

not against automating anything

maybe we should take this opportunity to move some tests (which don't require a local petstore server) to github workflows instead

cc @macjohnny