search

keymanapp / lexical-models

Lexical language models for predictive text
MIT License
13 stars 38 forks source link

[sil.brb.brao]: brao wordlist enhancement #279

Closed Meng-Heng closed 1 month ago

Meng-Heng commented 1 month ago

Continuing from https://github.com/keymanapp/lexical-models/pull/166

keyman-server commented 1 month ago

Thank you for your pull request. You'll see a "build failed" message until the Keyman team has reviewed the pull request and manually initiated the build process.

Every change committed to this branch will become part of this pull request. When you have finished submitting files and are ready for the Keyman team to review this pull request, please post a "Ready for review" comment.

DavidLRowe commented 1 month ago

Some minor changes:

The copyright statement in LICENSE.md is good: Copyright © 2021-2024 SIL Global

Other places where the copyright statement is included should either match what is in LICENSE.md or else omit the year and just use: Copyright © SIL Global. With the year removed from the copyright statement, these files don't need to be changed when a new version is released. In fact, in the case of the README.md, readme.htm, and welcome.htm files, you could omit the copyright line completely.

I'd also suggest removing the Version number line from README.md so that that file doesn't need to be updated when a new version is released. And perhaps the description could be simply: This model was created based on a wordlist. If you really want to include the word count, that could go into the HISTORY.md file, which is updated for each new version.

Here's one additional suggestion, which you are free to ignore if you don't want to implement it. It seems also that the text (Khmer?) in readme.htm and welcome.htm include the word count. Is it possible to phrase this in such a way that it doesn't have the exact count (and therefore doesn't have to be updated for every version)? Perhaps something like: "This model is based on a word list. Later versions will add more words to the list." (Or even just omit that sentence.)

I'm not clear how PR #166 relates to this one. Have those changes been included in this PR?

Meng-Heng commented 1 month ago

Thank you @DavidLRowe for the thorough inspection. Could you have a look if I have everything now?

I'm not clear how PR https://github.com/keymanapp/lexical-models/pull/166 relates to this one. Have those changes been included in this PR?

I was continuing the work from 166, the changes from there is listed in the HISTORY.md section 1.1. Do we need the 166 to be approved first for this PR?