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

Add term tags to {tags} Anki field #1464

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

khaitruong922
Copy link

@khaitruong922 khaitruong922 commented Oct 6, 2024

Fixes #1454

@khaitruong922 khaitruong922 requested a review from a team as a code owner October 6, 2024 17:08
@khaitruong922 khaitruong922 marked this pull request as draft October 6, 2024 17:11
@MarvNC
Copy link
Member

MarvNC commented Oct 6, 2024

Maybe it should be a separate handlebar?

@khaitruong922 khaitruong922 marked this pull request as ready for review October 6, 2024 18:38
@khaitruong922
Copy link
Author

Maybe it should be a separate handlebar?

We can expose the definition.termTags as a separate handlebar. If people only want to have either type of tags then it can be useful but I think it's kinda overkill and may cause confusion between {term-tags} and {definition-tags}.

@Kuuuube thoughts?

@Kuuuube
Copy link
Member

Kuuuube commented Oct 6, 2024

Best to avoid renaming existing handlebars if it can be avoided. Not sure what the use case for the current tags is to know if this matters.

@khaitruong922
Copy link
Author

Current {tags} field only returns tags from term definitions (for example n, v5, abbr) but not the tags of the term itself (mostly from JMDict) like news20k, ichi, rare tag, high priority term tag, etc.

Currently there is no handlebar to retrieve the term tags, so I add those into the {tags} anki field. I think use cases which requires a separate handlebar are quite limited.

@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/anki The issue or PR is related to Anki integration labels Oct 6, 2024
Copy link

codspeed-hq bot commented Oct 13, 2024

CodSpeed Performance Report

Merging #1464 will not alter performance

Comparing khaitruong922:tags-headword (ce314de) with master (73793a4)

Summary

✅ 3 untouched benchmarks

@jamesmaa
Copy link
Collaborator

@Kuuuube could you approve if you feel like PR is mergeable?

I feel like if we think {tags} should include both term and definition tags, then let's not worry about existing user expectations and Hyrum's Law we really don't have an API contract with our users nor a way to roll out these changes without violating user behaviors sometimes. I don't think that should stop up from doing the right thing if we believe it's the right thing

@stephenmk
Copy link

stephenmk commented Oct 14, 2024

Term tags serve a different purpose than definition tags. The definition tags are displayed once at the top of a yomitan term, and there could be one or many different sets of term tags displayed underneath. I'm not sure how exactly this new setup works (nobody has posted anki screenshots), but if the definition tags are repeated in each of these term tag sets, I imagine it would look very cluttered and I expect many users would be unhappy.

(Edit: I got "term" and "definition" reversed)

(Edit 2: I looked a bit more and I was actually thinking of the glossary handlebar, so never mind)

@Kuuuube
Copy link
Member

Kuuuube commented Oct 14, 2024

I'm fine with it as long as it works as it is said (tests look fine). I haven't had the chance to test this myself.

As for users depending on this, this is for sure one of the least used handlebars we have. I've never seen anyone use it.

@Kuuuube Kuuuube added this pull request to the merge queue Oct 14, 2024
Merged via the queue into yomidevs:master with commit 26e843d Oct 14, 2024
11 checks passed
4890A added a commit to 4890A/yomitan that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/anki The issue or PR is related to Anki integration kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

{tags} handlebar doesnt return term tags
6 participants