Skip to content

Conversation

@shaohuzhang1
Copy link
Contributor

feat: i18n

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Jan 14, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Jan 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

raise ValueError(_("Parameter value error: The uploaded audio lacks file_id, and the audio upload fails."))
return self.execute(**self.node_params_serializer.data, **self.flow_params_serializer.data,
app_document_list=app_document_list, app_image_list=app_image_list,
app_audio_list=app_audio_list,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code provided is mostly clear and free of major issues. Here are a few minor points to consider:

  1. In the error message, replace "Uploaded image lacking file_id" with "Uploaded audio lacks file_id".

  2. As raise ValueError() suggests an issue in parameter values during upload, it might be beneficial to log this warning as well within the method _run using something like:

logger.warning("Missing 'file_id' in uploaded media.")

Here's your refined version of the code snippet:

@@ -63,7 +63,7 @@ def _run(self):
                 app_audio_list[1:])
             for audio in app_audio_list:
                 if 'file_id' not in audio:
-                    raise ValueError(_("Parameter value error: Uploaded image lacks file_id, and the audio upload fails."))
+                    raise ValueError(_("Parameter value error: Uploaded audio lacks file_id, and the audio upload fails."))
                 logger.warning("Missing 'file_id' in uploaded media.")  # Optional logging suggestion
     return self.execute(**self.node_params_serializer.data, **self.flow_params_serializer.data,
                             app_document_list=app_document_list, app_image_list=app_image_list,
                             app_audio_list=app_audio_list,

These changes will ensure consistency across parameter name usage, handle missing file_id, and provide additional debugging information through logging.

message=_("The field only supports string|int|dict|array|float"), code=500)
])
source = serializers.CharField(required=True, error_messages=ErrMessage.char(_("source")), validators=[
validators.RegexValidator(regex=re.compile("^custom|reference$"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few issues in the provided code:

  1. The code parameter in the regular expression validator has two different values (500) and does not match the default HTTP status codes (e.g., 400, 404). Consider using valid HTTP status codes.

  2. The translation key used in the error messages is incorrect. Using _("field") instead of "field" might lead to unexpected results when translations are applied. Use double quotes within the format string for consistency.

Here's the corrected version of the code with these changes:

@@ -25,7 +25,7 @@ class InputField(serializers.Serializer):
     is_required = serializers.BooleanField(required=True, error_messages=ErrMessage.boolean("Is this field required"))
     type = serializers.CharField(required=True, error_messages=ErrMessage.char("type"), validators=[
         validators.RegexValidator(
             regex=re.compile("^string|int|dict|array|float$"),
-            message=_("The field only supports string|int|dict|array|float"), code=500
+            message="The field only supports string|int|dict|array|float", code=400
         )
     ])
     source = serializers.CharField(required=True, error_messages=ErrMessage.char("source"), validators=[
         validators.RegexValidator(
             regex=re.compile("^custom|reference$")

This should resolve both identified issues in the code.

error_messages=ErrMessage.float(_('similarity')))
search_mode = serializers.CharField(required=True, validators=[
validators.RegexValidator(regex=re.compile("^embedding|keywords|blend$"),
message=_("The type only supports register|reset_password"), code=500)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an analysis of the provided DatasetSettingSerializer code:

Irregularity/Suggestions

  1. Variable Name Change:

    • The variable name from similarity to _similarity is unnecessary and should be removed. Variable names should reflect their intended purpose clearly.
  2. Field Definition Errors:

    • In the first similari field definition (similarity = ...), there seems to be a duplication of validation codes.
      similarity = serializers.FloatField(required=True, max_value=2, min_value=0,
          error_messages=ErrMessage.float(_("Reference segment number")),
      Replace _float( with float.
  3. Error Message Translation:

    • Some translations are not fully specified:
      reference_segment_number`
      blend$
  4. Regular Expression for Validation:

    • The regex pattern might need adjustments based on the expected input values:
      regex=re.compile("^embedding|keywords|blend$")

Optimizations and Corrections

  1. Remove Duplicated Error Messages:

    similarity = serializers.FloatField(
        required=True,
        max_value=2,
        min_value=0,
        error_messages={
            "max_value": ErrMessage.float("Maximum value exceeded for similarity"),
            "min_value": ErrMessage.float("Minimum value not met for similarity"),
        },
    )
  2. Correctly Defined Regular Expression:
    Ensure the correct characters are used in the regular expression if needed:

    validator_regex = re.compile(r"^(embedding|keywords|blend)$")
    search_mode = serializers.CharField(
        required=True,
        validators=[validators.RegexValidator(regex=validator_regex)],
        message=_("The type only supports embedding|keywords|blend", code=500),
    )

These changes ensure that the serializer functions correctly without unnecessary duplication or errors.

@wxg0103 wxg0103 merged commit 255b4fd into main Jan 14, 2025
4 checks passed
@wxg0103 wxg0103 deleted the pr@main@feat_i18n branch January 14, 2025 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants