Skip to content

Filtering for the handlebars rank vs occurence#2373

Open
SHIRO-Suit wants to merge 4 commits intoyomidevs:masterfrom
SHIRO-Suit:filtered-freq-by-respective-field
Open

Filtering for the handlebars rank vs occurence#2373
SHIRO-Suit wants to merge 4 commits intoyomidevs:masterfrom
SHIRO-Suit:filtered-freq-by-respective-field

Conversation

@SHIRO-Suit
Copy link
Copy Markdown

Fix for #2322
Makes both average rank and harmonic rank use only rank frequencies. And average occurence and harmonic occurence use only occurence frequencies.
This prevents having a 10,000 rank frequency word but appears only once in the book and being averaged at 5,000 because of that. completely thowing off anki sort feature.

Makes both average rank and harmonic rank use only rank frequencies.
And average occurence and harmonic occurence use only occurence frequencies.
@SHIRO-Suit SHIRO-Suit requested a review from a team as a code owner April 3, 2026 21:36
Copy link
Copy Markdown
Member

@Kuuuube Kuuuube left a comment

Choose a reason for hiding this comment

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

This is not how handlebars changes work. You cannot change an existing upgrade file. You must create a new one, bump the overall yomitan settings version, and run a handlebars apply within that.

Also not all freq dicts have a frequencyMode in them. Can't break handling of them.

@SHIRO-Suit
Copy link
Copy Markdown
Author

SHIRO-Suit commented Apr 6, 2026

Ok, I agree this PR is quite extreme completely removing them if a single dictionary has this field. Is it what you meant by breaking their handling ? is it preferable to have the legacy dictionaries being counted in the calculations of both rank and occurence then?
Edit: thinking about it, they should just be treated like they were before. only dicts of explicitly opposite mode should be filtered.
Edit 2: the reason I was reluctant is that I was afraid it would keep breaking the average in my case, but the whole point of this PR is to have yomitan handle all cases so that I just have to edit the dicts that cause issues

@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 Apr 6, 2026
@SHIRO-Suit
Copy link
Copy Markdown
Author

I made changes, I reverted the v24, and set up a new v75 properly I think.
I changed the handling in getfrequencynumbers() so that wrong frequency is false when the field is undefined or null, this way legacy dictionaries act the same as before. I tested it and the averages seem correct.

Had to run test:unit:write to regenerate the expected json for dictionary-data.js because one test would fail since there are new fields in the class and it generates the object from them to do the assertion. I don't know if it's the right thing to do, let me know.

@SHIRO-Suit SHIRO-Suit requested a review from Kuuuube April 10, 2026 12:57
Signed-off-by: Searaw <43580252+SHIRO-Suit@users.noreply.github.com>
Copy link
Copy Markdown
Member

@Kuuuube Kuuuube left a comment

Choose a reason for hiding this comment

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

From a quick look.

Comment on lines +505 to +508
/**
* How the frequency should be interpreted.
*/
frequencyMode?: 'occurrence-based' | 'rank-based' | null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Image

Comment on lines +39 to +40
/** How the frequency should be interpreted. */
frequencyMode?: 'occurrence-based' | 'rank-based';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can import the type with import type * as DictionaryData from './dictionary-data'; and use DictionaryData.FrequencyMode.

- replaced the hard coded frequencymode types in both d.ts files and JSDoc
- Added missing end of file newline for the new handlebars template.
- Removed the method that was extracted because there was duplicate usage for wrongheadwordindex, and it's no longer there.
- made the frequencymode for kanjifrequency required instead since at this point it shouldn't be undefined anymore.
@SHIRO-Suit SHIRO-Suit requested a review from Kuuuube April 13, 2026 10:04
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.

2 participants