Skip to content

feat(translator): allow a translator to take multiple tags#926

Merged
ksqsf merged 1 commit intorime:masterfrom
ksqsf:translator-tags
Sep 21, 2024
Merged

feat(translator): allow a translator to take multiple tags#926
ksqsf merged 1 commit intorime:masterfrom
ksqsf:translator-tags

Conversation

@ksqsf
Copy link
Member

@ksqsf ksqsf commented Aug 14, 2024

Pull request

Feature

Allow script_translator and table_translator to take multiple tags.

translator:
  tags: [abc, tag1, tag2, tag3]

To keep compatibility:

  1. if there is only tag: tags is set to that tag
  2. if there is only tags: tags is set to that list
  3. if there are both tag and tags: tag is the first element in the list, and tags is understood similar to extra_tags

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

@ksqsf ksqsf changed the title feat(translator_commons): allow a translator to take multiple tags feat(translator): allow a translator to take multiple tags Aug 14, 2024
const string& delimiters() const { return delimiters_; }
const string& tag() const { return tag_; }
void set_tag(const string& tag) { tag_ = tag; }
vector<string> tags() const { return tags_; }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not const vector<string>& due to a compilation eror in librime-lua which I don't know how to solve.

@ksqsf ksqsf requested a review from a team August 15, 2024 08:56
config->GetList(ticket.name_space + "/disable_user_dict_for_patterns"));
string tag;
if (config->GetString(ticket.name_space + "/tag", &tag)) {
// replace the first tag, and undersand /tags as extra tags
Copy link
Member

Choose a reason for hiding this comment

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

is undersand a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, fixed.

also fixed a case where the invariant could be broken.

@ksqsf ksqsf merged commit d47a812 into rime:master Sep 21, 2024
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.

2 participants