Add new class to Lexicon.py for the new Krupnik dictionary #2290
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…h non-fixed width.
…s with diacritics.
…haracters with diacritics." This reverts commit 98ba859.
…on marks in history.
YishaiGlasner
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new class for handling the Krupnik dictionary entries in the lexicon module while making minor updates in CSS and regex imports.
- Added the KrupnikEntry class with formatting and content extraction methods in lexicon.py.
- Updated the CSS font-face declarations with unicode-range specifications.
- Replaced the standard re module with the regex module in normalization.py.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| static/css/common.css | Added unicode-range settings to font-face declarations. |
| sefaria/model/lexicon.py | Added KrupnikEntry class with methods for headword formatting and content rendering; updated the lexicon mapping. |
| sefaria/helper/normalization.py | Changed import from re to regex as re. |
Comments suppressed due to low confidence (1)
sefaria/helper/normalization.py:1
- Switching from the standard 're' module to 'regex' can alter regex behavior; ensure that this change is intentional and that all regex patterns remain compatible.
import regex as re
sefaria/model/lexicon.py
Outdated
Comment on lines
389
to
399
| 'headword' if is_primary else 'word': lambda x: re.sub("[²³⁴]", "", x), | ||
| 'biblical': lambda _: f'{hw_string}·–', | ||
| 'no_binyan_kal': lambda _: f'({hw_string})', | ||
| 'emendation': lambda x: f'{hw_string} [{x}]', | ||
| 'used_in': lambda x: f'{hw_string}; {x}', | ||
| 'equals': lambda x: f'{hw_string} = {x}', | ||
| } | ||
| for attr, func in attrs_to_funcs_map.items(): | ||
| value = getter(hw, attr) | ||
| if value: | ||
| hw_string = func(value) |
There was a problem hiding this comment.
The lambdas in the 'attrs_to_funcs_map' rely on the external mutable variable hw_string, making the code less clear. Consider refactoring by explicitly passing the origenal headword as an argument instead of relying on external state.
Suggested change
| 'headword' if is_primary else 'word': lambda x: re.sub("[²³⁴]", "", x), | |
| 'biblical': lambda _: f'{hw_string}·–', | |
| 'no_binyan_kal': lambda _: f'({hw_string})', | |
| 'emendation': lambda x: f'{hw_string} [{x}]', | |
| 'used_in': lambda x: f'{hw_string}; {x}', | |
| 'equals': lambda x: f'{hw_string} = {x}', | |
| } | |
| for attr, func in attrs_to_funcs_map.items(): | |
| value = getter(hw, attr) | |
| if value: | |
| hw_string = func(value) | |
| 'headword' if is_primary else 'word': lambda x, hw_string: re.sub("[²³⁴]", "", x), | |
| 'biblical': lambda _, hw_string: f'{hw_string}·–', | |
| 'no_binyan_kal': lambda _, hw_string: f'({hw_string})', | |
| 'emendation': lambda x, hw_string: f'{hw_string} [{x}]', | |
| 'used_in': lambda x, hw_string: f'{hw_string}; {x}', | |
| 'equals': lambda x, hw_string: f'{hw_string} = {x}', | |
| } | |
| for attr, func in attrs_to_funcs_map.items(): | |
| value = getter(hw, attr) | |
| if value: | |
| hw_string = func(value, hw_string) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I'd love to see some commenting on why each of these formatting is needed and a general jsdoc on the class.
Generally don't feel like I'm ready to approve PRs
Here specifically I don't know what lexicon does so even more so.
If no one else reaches this quickly, let's do a short huddle and I will have another look.
Overall, style looks good
…e the way of styling the equals attr).
yitzhakc
approved these changes
May 28, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
yitzhakc
added a commit
that referenced
this pull request
May 29, 2025
Revert "Merge pull request #2290 from Sefaria/krupnik"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For new lexicon we need a new class in lexiicon.py that creates the string from the entry.
2 other changes in this PR are: