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

Added geminate consonants to Norwegian Bokmal phonelist #542

Merged
merged 5 commits into from
May 31, 2024

Conversation

fhallee
Copy link
Contributor

@fhallee fhallee commented May 29, 2024

Added long consonants Norwegian Bokmal (nob) phonelist, as they were missing.

Closes #532

@kylebgorman
Copy link
Collaborator

The change to the phonelist itself looks good. Please follow steps 4-7 here and the tests you see above ought to pass.

I am assuming that segments like /h/ are excluded on the grounds they are (never?) geminates in this language.

@fhallee
Copy link
Contributor Author

fhallee commented May 30, 2024

I am assuming that segments like /h/ are excluded on the grounds they are (never?) geminates in this language.

That’s right, I only considered consonants for which I found evidence, either online or in the data itself, that they can be geminates. /h/ was not one of these. Unfortunately, there wasn't a lot of detailed information online. Many sources simply mentioned the possibility of "long consonants” and their relationship to short vowels.

@kylebgorman
Copy link
Collaborator

That’s right, I only considered consonants for which I found evidence, either online or in the data itself, that they can be geminates. /h/ was not one of these.

That’s fine, proceed with that.

@@ -0,0 +1,4 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please run git checkout -- unscraped.json (in this directory, or use the full path from another directory). This doesn't need checked in.

@@ -460,7 +460,8 @@
"latn": "Latin",
"hira": "Hiragana",
"bopo": "Bopomofo",
"hani": "Han"
"hani": "Han",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please run git checkout -- languages.json (in this directory, etc.) on this file. It doesn't need checked in or modified here, even if the procedures caused it to be. (I think that's a very minor bug for us to fix separately.)

@kylebgorman
Copy link
Collaborator

Reverting those two files (as suggested above) and then rerunning ./postprocess in both the scrape and phones directory ought to get the tests passing again---try it out.

@kylebgorman
Copy link
Collaborator

I realize I mispoke. I think that git checkout won't undo a change you've already committed, it will just undo an uncommitted change? That is why languages.json and unscraped.json are still in this PR. I had to research how to do this. The internet says to try the newly-added (as of late 2019) verb restore. That is: git restore myfile will restore myfile to its state at HEAD. After committing and pushing, you'll know it worked if those files are no longer listed in the PR on GitHub.

@fhallee
Copy link
Contributor Author

fhallee commented May 31, 2024

Restore didn’t seem to do anything, but I was able to remove the two files from the PR by reverting the files to the commit before I made the changes using git checkout <commit-hash> <file-name> and then committing those changes

@kylebgorman
Copy link
Collaborator

There is now a different, later error that the tests are failing on, but it's one completely unrelated to what you did so I give you a dispensation to submit; I'll resolve that issue separately. Let me know if you're ready for me to merge.

@fhallee
Copy link
Contributor Author

fhallee commented May 31, 2024

Yep, ready!

@kylebgorman
Copy link
Collaborator

Before I merge this I'm going to edit your PR description to include the string "Closes #532". This tells GitHub that the PR, once merged, should be linked with that issue, and should mark it as closed.

@kylebgorman kylebgorman merged commit cf3bccf into CUNY-CL:master May 31, 2024
4 of 10 checks passed
@fhallee fhallee deleted the nob branch May 31, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[nob] overly restrictive phonelist
2 participants