Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow numbers and symbols on yaml context #309

Conversation

nkarab
Copy link
Contributor

@nkarab nkarab commented Aug 31, 2023

Problem and/or solution

So far, we've been able to include context in YAML files (YAML_GENERIC, YML i18n) tx doc by utilizing custom YAML tags .
For instance:

string: !context translation 

However, our Yaml parser for the context only allows alphabetic characters, zeros, and underscores. For instance:

a_string_without_context: !context1 translation  # context -> None
a_string_with_context: !context_smth translations  # context -> !context_smth

We have now extended support to include numbers and a limited set of special symbols namely (- _ : .).

Additionally, the context is stored in our database with the exclamation mark, and we aim to remove it.

backwards compatibility
slack conversation about it: https://transifex.slack.com/archives/C01BHGRAGH1/p1693309705338759

How to test

Upload a YML i18n and a YAML_GENERIC resource file that includes custom tags featuring numbers or special symbols.

en:
  context_string: !he:fd94_fd-la. "context string"
  verbim_context_string: !<!context545qa> "verbim context string"
  context_on_nested_map:
    first: !first_context:54KJFLA95KJ4 "context in nested map"
    second: !second_context:FDKJ40DK "context in nested map"

We expect these contextes:

string key context
context_string he:fd94_fd-la.
verbim_context_string context545qa
context_on_nested_map.first first_context:54KJFLA95KJ4
context_on_nested_map.second second_context:FDKJ40DK

We expect a downloaded file for the source language like this:

en:
  context_string: !he:fd94_fd-la. "context string"
  verbim_context_string: !context545qa "verbim context string"
  context_on_nested_map:
    first: !first_context:54KJFLA95KJ4 "context in nested map"
    second: !second_context:FDKJ40DK "context in nested map"

We expect for the translation language a file like this:

el:
  context_string: !he:fd94_fd-la. ""
  verbim_context_string: !contex545qa ""
  context_on_nested_map:
    first: !first_context:54KJFLA95KJ4 ""
    second: !second_context:FDKJ40DK ""
    

Staging: https://yamlcntxt.stg.transifex.net/

Reviewer checklist

Code:

  • Change is covered by unit-tests
  • Code is well documented, well styled and is following best practices
  • Performance issues have been taken under consideration
  • Errors and other edge-cases are handled properly

PR:

  • Problem and/or solution are well-explained
  • Commits have been squashed so that each one has a clear purpose
  • Commits have a proper commit message according to TEM

@nkarab nkarab marked this pull request as ready for review August 31, 2023 12:25
@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from 1dabb16 to 590e19c Compare September 1, 2023 11:25
Copy link
Contributor

@fathineos fathineos left a comment

Choose a reason for hiding this comment

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

Let's add a test case with !! as well

@fathineos
Copy link
Contributor

Let's add a test case with !! as well

ignore my comment, I see that we already have such tests in place

@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from c8ea0bf to 01753fe Compare September 5, 2023 10:10
@nkarab nkarab requested a review from kbairak September 6, 2023 07:45
kbairak
kbairak previously approved these changes Sep 6, 2023
@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from 0de4746 to 1c8c021 Compare September 8, 2023 09:45
@nkarab nkarab requested a review from kbairak September 12, 2023 09:15
kbairak
kbairak previously approved these changes Sep 12, 2023
Copy link
Member

@kbairak kbairak left a comment

Choose a reason for hiding this comment

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

Looks great! The comment is optional.

Built-in types, indicated by a `!!` prefix, will not be matched. We
can't preserve the information whether a built-in tag like `!!str` was
used for a value since the PyYAML library will tag such entries with
the built-in identifier. For example `tag:yaml.org,2002:str`, not
`!!str`.
"""
return re.match(ensure_unicode(r'^[\![a-zA-Z_]*]*$'),

return re.match(ensure_unicode(r'^\![a-zA-Z0-9_:.\-]*$'),
Copy link
Member

Choose a reason for hiding this comment

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

Why not \w instead of a-zA-Z0-9?

tag,
re.IGNORECASE)

Copy link
Member

Choose a reason for hiding this comment

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

The linter is not going to like this.

@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from 1c8c021 to eea8c3e Compare September 12, 2023 09:22
@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from eea8c3e to 6b0bdd4 Compare September 12, 2023 09:31
@nkarab nkarab force-pushed the CHAM-586-allow-number-and-symbols-in-the-context-field-for-yaml-files branch from 6b0bdd4 to b1c8d33 Compare September 12, 2023 09:32
@nkarab nkarab requested a review from kbairak September 12, 2023 09:35
@nkarab nkarab merged commit b765d08 into devel Sep 12, 2023
2 checks passed
@txsentinel txsentinel mentioned this pull request Sep 12, 2023
@asgouros-tx asgouros-tx changed the title CHAM-586 Allow numbers and symbols on yaml context Allow numbers and symbols on yaml context Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants