-
Notifications
You must be signed in to change notification settings - Fork 113
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 phonetic transcriptions term meta type #434
add phonetic transcriptions term meta type #434
Conversation
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
Are you referring to the JSON config files? It's fine to be in this since it's close enough related I'd say. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some initial thoughts.
Assuming you're based off latest master, you can run |
npm run test-code-write
> [email protected] test-code-write
> vitest run --config test/data/vitest.write.config.json
RUN v0.34.6 /media/stefan/DATA/EDesktop/Projekti/Coding/yezichak
❯ test/dictionary-data.write.js (0)
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
FAIL test/dictionary-data.write.js [ test/dictionary-data.write.js ]
ReferenceError: TransformStream is not defined
❯ node_modules/@zip.js/zip.js/lib/core/constants.js:238:19
❯ ext/js/dictionary/dictionary-importer.js:2:31
1| /*
2| * Copyright (C) 2023 Yomitan Authors
| ^
3| * Copyright (C) 2020-2022 Yomichan Authors
4| *
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
Test Files 1 failed (1)
Tests no tests
Start at 22:57:17
Duration 2.15s (transform 626ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 211ms) |
Try rebuilding libraries: |
No change. |
updated Node to v20, manually deleted node_modules, npm install - now it works |
🤔 Edit - nevermind, you commented above.
|
I think overall this looks good. The only thing that I think would be good to add is a/some test data in |
I've added some test data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much both of you for polishing this PR 🙏🙏
part 2 of #422. Adds a 3rd term meta type for IPA transcriptions.
The types could maybe be improved somehow, there is a ts error in
anki-note-data-creator.js
because of the union type.Also, I missed 2 references to
dictionary-data-util.js
in #429, should i fix that in this pr or make another?