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 phonetic transcriptions term meta type #434

Merged
merged 24 commits into from
Dec 28, 2023

Conversation

StefanVukovic99
Copy link
Collaborator

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?

@StefanVukovic99 StefanVukovic99 requested a review from a team as a code owner December 23, 2023 14:09
Copy link

github-actions bot commented Dec 23, 2023

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

@toasted-nutbread
Copy link

Also, I missed 2 references to dictionary-data-util.js in #429, should i fix that in this pr or make another?

Are you referring to the JSON config files? It's fine to be in this since it's close enough related I'd say.

Copy link

@toasted-nutbread toasted-nutbread left a 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.

ext/js/data/sandbox/anki-note-data-creator.js Outdated Show resolved Hide resolved
types/ext/dictionary-database.d.ts Outdated Show resolved Hide resolved
types/ext/dictionary-database.d.ts Outdated Show resolved Hide resolved
ext/js/dictionary/dictionary-data-util.js Outdated Show resolved Hide resolved
ext/js/dictionary/dictionary-data-util.js Outdated Show resolved Hide resolved
ext/js/display/display-generator.js Outdated Show resolved Hide resolved
@toasted-nutbread
Copy link

Assuming you're based off latest master, you can run npm run test-code-write to fix the test data.

types/ext/dictionary-database.d.ts Outdated Show resolved Hide resolved
ext/js/display/display-generator.js Outdated Show resolved Hide resolved
@StefanVukovic99
Copy link
Collaborator Author

Assuming you're based off latest master, you can run npm run test-code-write to fix the test data.

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)

@toasted-nutbread
Copy link

Try rebuilding libraries: npm run build-libs

@StefanVukovic99
Copy link
Collaborator Author

Try rebuilding libraries: npm run build-libs

No change.

@StefanVukovic99
Copy link
Collaborator Author

Try rebuilding libraries: npm run build-libs

No change.

updated Node to v20, manually deleted node_modules, npm install - now it works

@toasted-nutbread
Copy link

toasted-nutbread commented Dec 24, 2023

🤔

Edit - nevermind, you commented above.

~\yomitan>npm ci
npm WARN deprecated @types/[email protected]: This is a stub types definition. handlebars provides its own type definitions, so you do not need this installed.      
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated

added 492 packages, and audited 493 packages in 8s

108 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

~\yomitan>npm run build-libs

> [email protected] build-libs
> node ./dev/bin/build-libs.js


~\yomitan>npm run test-code-write

> [email protected] test-code-write
> vitest run --config test/data/vitest.write.config.json


 RUN  v0.34.6 ~/yomitan

 ✓ test/dictionary-data.write.js (1) 1386ms
   ✓ Write dictionary data expected data 1385ms

 Test Files  1 passed (1)
      Tests  1 passed (1)
   Start at  17:10:41
   Duration  4.80s (transform 543ms, setup 0ms, collect 830ms, tests 1.39s, environment 0ms, prepare 96ms)


~\yomitan>git diff --shortstat
 3 files changed, 893 insertions(+), 31 deletions(-)

djahandarie
djahandarie previously approved these changes Dec 25, 2023
.gitignore Outdated Show resolved Hide resolved
@toasted-nutbread
Copy link

I think overall this looks good. The only thing that I think would be good to add is a/some test data in test/data/translator-test-inputs.json and test/data/dictionaries/valid-dictionary1/term_meta_bank_1.json.

@StefanVukovic99
Copy link
Collaborator Author

I've added some test data.

Copy link
Collaborator

@djahandarie djahandarie left a 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 🙏🙏

@djahandarie djahandarie added this pull request to the merge queue Dec 28, 2023
Merged via the queue into yomidevs:master with commit fc2123a Dec 28, 2023
5 checks passed
@djahandarie djahandarie added the kind/meta The issue or PR is meta label Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/meta The issue or PR is meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants