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

Arabic toponym checks #349

Merged
merged 8 commits into from
Jun 26, 2023
Merged

Arabic toponym checks #349

merged 8 commits into from
Jun 26, 2023

Conversation

stervel
Copy link
Contributor

@stervel stervel commented May 30, 2023

This is my first pr, I might need a hand if there's any mistake.

Copy link
Owner

@hmlendea hmlendea left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have just a few changes I'd make (see comments), but other than that it's great!!

locations.xml Outdated
@@ -15363,7 +15363,7 @@
</GameIds>
<Names>
<Name language="Alemannic" value="Die autonomi Gmäinschaft Aragonie" />
<Name language="Arabic" value="Araghún" />
<Name language="Arabic" value="Saraqusṭaẗ" comment="There only exists Taifa (Duchy) of Saraqusṭaẗ in history, if Aragon is preferable, use 'Araġūn'" />
Copy link
Owner

Choose a reason for hiding this comment

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

I would stick with Araġūn since the title is *_aragon. I generally try to just stick to names and refrain from fixing the game's inaccuracies, as that can be done through separate mods

locations.xml Outdated
@@ -156264,6 +156264,7 @@
<LocationId>badajoz</LocationId>
</FallbackLocations>
<Names>
<Name language="Arabic" value="Baṭalyūs" comment="Based on the name of Taifa(?)" />
Copy link
Owner

Choose a reason for hiding this comment

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

This should be placed under badajoz. This location (lusitania) already inherits names from badajoz when its own are not available - which is just the case for Arabic.

Also, badajoz already has an Arabic name "Baṭalyaws", you can just update that one instead, as yours has the correct diacritics.

Copy link
Owner

Choose a reason for hiding this comment

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

BTW: The locations from which the names are inherited are defined under <FallbackLocations></FallbackLocations>

locations.xml Outdated
@@ -318029,6 +318031,7 @@
<LocationId>lleida</LocationId>
</FallbackLocations>
<Names>
<Name language="Arabic" value="Lāridaẗ" />
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be under location lleida, which this location (lleida_region) already inherits names from.
Unless it's a different form specific to the region of Lleida, in which case it's fine to have it here.

locations.xml Outdated
@@ -318999,6 +319002,7 @@
<LocationId>coruna</LocationId>
</FallbackLocations>
<Names>
<Name language="Arabic" value="Qarǧiṭaẗ" comment="found in 4118Htm" />
Copy link
Owner

Choose a reason for hiding this comment

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

This location (coruna_county) already inherits names from location coruna, so no need to define it in both places. If it's used for Coruña generically, put it under coruna, if it's specific to just the county/region, put it under coruna_county.

@stervel
Copy link
Contributor Author

stervel commented May 31, 2023

Hello. Thanks, it was a pleasure! I really need to familiarize myself with the code structure more.

A question since I'm new to this. What will happen if I click the "Resolve conversation" button below each comments? I do think I should make a new commit, but I also think those above are like "revision approval" sections? Which one is correct?


EDIT: Nvm, I get it now.

@hmlendea hmlendea merged commit d79a97a into hmlendea:master Jun 26, 2023
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.

2 participants