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 fluent function for indefinite article #2805

Merged
merged 2 commits into from
Jun 6, 2022

Conversation

mirrorcult
Copy link
Contributor

Mostly stolen from here: https://stackoverflow.com/a/8044744

Works on either string or entity loc values. For entity just takes it from metadata name

Proof it works:

image

image

(these two are just tested, not actually implementing that)
image

image

Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

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

So one concern I have about this is that this is a very English-centric approach. Now obviously you'd have to change this code for other languages, but there are many languages where this type of guesswork isn't possible. I'm just wondering if it would be better to implement this in a more fundamental part of the localization system, somehow.

@mirrorcult
Copy link
Contributor Author

mirrorcult commented May 16, 2022

So one concern I have about this is that this is a very English-centric approach. Now obviously you'd have to change this code for other languages, but there are many languages where this type of guesswork isn't possible. I'm just wondering if it would be better to implement this in a more fundamental part of the localization system, somehow.

we already have many fluent functions that are english-centric that are widely used, the solution remie had was to just have locale-specific functions, which is fine, but not particularly needed (in this PR) right now

@PJB3005
Copy link
Member

PJB3005 commented May 16, 2022

That's not what I mean. The problem is that I'm concerned we should maybe have a thorough way of passing through the indefinite article (or any other such data) in a general purpose manner, instead of having an English-specific workaround to guess it. Then we could declare whether something is indefinite or not in the localization files and it would pass through.

This would probably be necessary for many languages. For example in Dutch the equivalent of "the" is either "het" or "de" depending on the word, and there's no rhyme or reason to it. All interpolated nouns would basically need to be annotated somehow.

@PJB3005
Copy link
Member

PJB3005 commented May 16, 2022

Another instance where this would be necessary is noun genders in French.

@mirrorcult
Copy link
Contributor Author

im gonna be real i have no idea how that would be accomplished in any way. for entities you could just use an attribute on the grammar component (like this has an override for) but how are you at all supposed to pass that data in with just, strings? especially if those are just strings defined in YAML?

the easiest way to do what you want with dutch IMO is to literally just have a big list somewhere of what nouns correspond to what article (if it is that random, idk dutch) and then have the function just lookup that

@wrexbe
Copy link
Contributor

wrexbe commented May 16, 2022

Not a cool programming solution, but it's possible to reword sentences so it doesn't matter
(p)assistant will be selected if your preference is unavailable.
There is no way to deconstruct the (not) APC

@PJB3005
Copy link
Member

PJB3005 commented May 16, 2022

That, too, is a hack that probably isn't gonna work long-term. Try using English without "the".

@wrexbe
Copy link
Contributor

wrexbe commented May 16, 2022

What about putting all these functions behind some kind of ILanguageAnnotationHandler, and make a DefaultEnglishHandler. Some of these annotations won't even make sense depending on the language.

@PJB3005
Copy link
Member

PJB3005 commented May 16, 2022

Current status has been "let downstreams help us out figuring it out". If need be I could do some work to add more exemplary Dutch support as a test case though.

@PJB3005 PJB3005 merged commit 3941930 into space-wizards:master Jun 6, 2022
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.

3 participants