Improve Identity server_default rendering for Decimal values

[NotCarlosSerrano]

Changes

Improve handling of Identity server defaults when rendering columns.

Previously, Identity attributes containing Decimal values (e.g. start, increment, minvalue, maxvalue) were not rendered correctly. This change inspects the Identity parameters and explicitly renders non-default values. When a value is a Decimal, it is now emitted as decimal.Decimal(...) and the required module import is added.

This ensures generated models correctly represent database identity configurations that rely on Decimal values.

Fixes #.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

• You've added tests (in tests/) which would fail without your patch
• You've added a new changelog entry (in CHANGES.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

[coveralls]

coverage: 97.832% (-0.03%) from 97.861%
when pulling d761fc1 on NotCarlosSerrano:fix/identity-decimal-serialization
into 9234437 on agronholm:master.

[agronholm]

Deleting the only test that covers this is hardly an acceptable solution to making the test suite pass.

[NotCarlosSerrano]

You're right,

I originally added a test expecting Decimal values to be rendered as decimal.Decimal(...), but after revisiting the implementation I changed the approach. Since the parameters of Identity (start, increment, minvalue, maxvalue, etc.) are defined as int | None, emitting Decimal values in the generated code didn't seem appropriate.

Instead, the implementation now normalizes Decimal values returned by some dialects to int. When I made that change, I removed the original test instead of updating it to reflect the new expected behavior.

I could add a dedicated test for the Decimal → int normalization if you think it's worth it, although the current Identity test should already cover the resulting behavior.

[agronholm]

@sheinbergon Thoughts on this one?

[sheinbergon]

@sheinbergon Thoughts on this one?

I'm not really liking this explicit attribute selection. Let's see if it's required or not

[agronholm]

@sheinbergon Thoughts on this one?

I'm not really liking this explicit attribute selection. Let's see if it's required or not

Same here, I'd like to avoid that if possible, as we're doing everywhere else.

[NotCarlosSerrano]

Good point, it would be better to avoid the explicit attribute selection, especially since that’s not how this is handled elsewhere in the generator. The intent here was only to render the non-default public Identity kwargs, not to introduce a separately maintained list.

I’ll take another pass and see if this can be implemented using the existing introspection-based approach instead. Does that direction make sense?

[sheinbergon]

Good point, it would be better to avoid the explicit attribute selection, especially since that’s not how this is handled elsewhere in the generator. The intent here was only to render the non-default public Identity kwargs, not to introduce a separately maintained list.

I’ll take another pass and see if this can be implemented using the existing introspection-based approach instead. Does that direction make sense?

Is there any problem with just getting all args, seeing if any non-null decimals are present and express them as ints?

[NotCarlosSerrano]

That sounds better to me too. I don’t think the special handling needs to be tied specifically to an explicit set of Identity attributes. I can keep the existing rendering approach and only normalize non-null Decimal values to int before rendering, so this stays consistent with the rest of the generator.

[sheinbergon]

That sounds better to me too. I don’t think the special handling needs to be tied specifically to an explicit set of Identity attributes. I can keep the existing rendering approach and only normalize non-null Decimal values to int before rendering, so this stays consistent with the rest of the generator.

Much better, 10x 🙏. can you add a test to cover this behavior?

[NotCarlosSerrano]

Done, I’ve pushed the changes including a test that covers the normalization of Decimal to int in Identity values.

I’m not sure if it’s a bit tricky to set the Decimal values directly in the test, but it seemed like the clearest way to simulate what might come from database reflection. Let me know if you’d like to see any additional cases covered.

[sheinbergon]

Done, I’ve pushed the changes including a test that covers the normalization of Decimal to int in Identity values.

I’m not sure if it’s a bit tricky to set the Decimal values directly in the test, but it seemed like the clearest way to simulate what might come from database reflection. Let me know if you’d like to see any additional cases covered.

Looks good. If the tests are generated the expected result (with respect to the issue you were trying to solve), it's all good

[agronholm]

A couple minor changes I'd like to see.

[agronholm]

LGTM. @sheinbergon feel free to merge unless you have more changes you'd like to see.

[NotCarlosSerrano]

Thanks for the merge! 🙌
Do you know if there’s an upcoming release planned that will include this change? I’d like to start using it once it’s available.

[sheinbergon]

@NotCarlosSerrano Just released 4.0.3. Feel free to make more contribution if you have the time!