I removed the optional part as we are already checking emptiness for key_url in https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/schema_registry/rules/encryption/hcvault/hcvault_driver.py#L53, and I think HcVaultKmsClient is supposed to be only created with a specified key_uri, according to the function doc

Not sure if this is considered a breaking change (if customers initializes HcVaultKmsClient directly in their code). @rayokota would love to hear your thoughts on this

Similar change as the one in hcvault_client.py

In practice the schema_str field should never be empty, and even it is I think it makes sense to raise the error to fail early here

Why is there a need for type ignoring here? Can we set the ref/referenced_schema type to avoid this?

In practice, subject and version of SchemaReference never be None, but probably for historical reason (or tech debt) they have been typed as optional. I think updating the types will be a breaking change

Here, get_version() requires non-empty subject and version, so that's why I added the type ignore there. Alternatively we can do:

 if ref.subject is None or ref.version is None:
 # maybe log something
 continue
 referenced_schema = await schema_registry_client.get_version(ref.subject, ref.version, True)

Maybe add a comment that 'type' in the dict is for the schema type and not any language type hinting? Seeing it might be confusing without context right next to a type ignore call.

I think we should spread this function args out per line if we're type commenting one. It read weird like this and I think is error prone to refactor bugs

This changed the logic here. Are we certain it's a correct / tested change?

_execute_rules_with_phase() already requires SerializationContext and subject to build RuleContext (

I refactored the name_strategy functions a bit: they have to follow the same function signature but SerializationContext is only required for some

Moving the indentation is intended to address the type issue of logical_cluster and identity_pool for building _AsyncOAuthClient: they must be non-empty, which we already check in line 280 and 284

This doesn't affect code logic: we check self.bearer_auth_credentials_source in {'OAUTHBEARER', 'STATIC_TOKEN'} for the outer block, and those if-else branches are for OAUTHBEARER and STATIC_TOKEN respectively

JsonSchema union type is either bool or dict. This was likely coped from avro.py

Corrected incomplete error message from 'after the at' to 'after the attempt to generate it'.

The elif condition check has been replaced with else without verifying all possible values. This assumes only 'OAUTHBEARER' and 'STATIC_TOKEN' are valid values for bearer_auth_credentials_source. Consider adding an explicit elif condition for 'STATIC_TOKEN' to make the code more maintainable and guard against unexpected values.

It's already the outer block if self.bearer_auth_credentials_source in {'OAUTHBEARER', 'STATIC_TOKEN'}:

The elif condition check has been replaced with else without verifying all possible values. This assumes only 'OAUTHBEARER' and 'STATIC_TOKEN' are valid values for bearer_auth_credentials_source. Consider adding an explicit elif condition for 'STATIC_TOKEN' to make the code more maintainable and guard against unexpected values.

It's already the outer block if self.bearer_auth_credentials_source in {'OAUTHBEARER', 'STATIC_TOKEN'}: (line 273)

The indentation is incorrect on line 867. The 'query['subject'] = subject_name' statement should be on its own line with proper indentation.

The indentation is incorrect on line 868. The 'query['subject'] = subject_name' statement should be on its own line with proper indentation.

Using a mutable default argument (empty list) is dangerous as it will be shared across all instances. Use None as default and initialize inside the function instead.

Missing space after comma between 'inline_tags' and 'field_transformer' arguments.

The logic for calculating new_version is confusing. When is_expired is False, new_version should be 1, but when is_expired is True, it should be dek.version + 1. The current conditional expression has this backwards - it increments when is_expired is True but sets to 1 when False. Consider: new_version = (dek.version + 1) if is_expired else 1

Calling .get() on a boolean value will raise AttributeError. After checking isinstance(schema, bool), the function should return immediately before attempting to call schema.get(). The early return is missing on line 213.

Adding a ValueError when ctx is None changes the API behavior. Previously, these functions accepted Optional[SerializationContext], and None would result in an AttributeError at line 94. The new explicit check is clearer, but this is a breaking change that could affect existing code that catches AttributeError. Consider documenting this as a breaking change.

[nitpick] Converting float to int with int() truncates the decimal. If cache_capacity is 100.9, it becomes 100. Consider using round() instead for more intuitive behavior, or document that fractional values are truncated.

[nitpick] This appears to be a pure formatting change (moving parameters to separate lines) unrelated to type hinting. Such formatting changes should typically be in a separate commit to keep type hinting changes focused.