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

Use an atom instead of a var for text dictionary. #41

Closed
wants to merge 3 commits into from
Closed

Use an atom instead of a var for text dictionary. #41

wants to merge 3 commits into from

Conversation

CharlesHD
Copy link
Contributor

Purpose : modification of the text source dictionary is easier with an atom and works both in Clojure and Clojurescript. It can change default text used internally.

The change is pretty light.

I also add a set-dictionary! function for providing a clear way of changing the dictionary. So if you want to swap atom for another construct it will be transparent for the API user.

@facundoolano
Copy link
Owner

did you tied it to see if it works with a game (perhaps the example-game)?

I worry about some translated strings that are used in maps and things like that, I remember at some point I had issues with setting the dictionary after the advenjure namespaces were initialized

@CharlesHD
Copy link
Contributor Author

I'll try on mine. I'll add some tests if you want using your different tense dictionaries.

@facundoolano
Copy link
Owner

Tests are not necessary I think, just want to make sure it works before merging

@CharlesHD
Copy link
Contributor Author

I tried both on jvm and javascript. Phrases I changed were indeed replaced in both cases.
I dunno if the example game is fully compatible with the current state of the lib (it uses version "0.6.3").

@facundoolano
Copy link
Owner

facundoolano commented Oct 19, 2017

the advenjure-v1 of the example game works with the current master of advenjure. I planned on merging there next time a publish an advenjure version (ideally when milestone v1 is finished)

@CharlesHD
Copy link
Contributor Author

I made a fork of the example game and set a branch dictionary-test where I set the dictionary to the present tense one. It works as expected on my manual checks.

You can clone my advenjure repo, install it to your local maven repo then clone the advenjure-example one to test it by yourself.

@CharlesHD
Copy link
Contributor Author

CharlesHD commented Oct 19, 2017

Oh. I got a case where it does not work as expected. The en_present dictionary does not cover the article case for "a" and "an" that the en_past dictionary covers. I think it's more an oversight in the en_present dictionary than anything else.

@facundoolano
Copy link
Owner

I just tested using the spanish translations, and this suffers the case I mentioned earlier. The english present that you tested is ok because it doens't redefine the verbs, but in spanish this happens:

@Bedroom [P:0 | M:1] > mirar alrededor
No sabía cómo hacer eso.

@Bedroom [P:0 | M:1] > look around
A smelling bedroom. There was an unmade bed near the corner and a door to the
north. A thin ladder leaded up. Just by the table, a trash bin. The floor was
scratched near the bed.

The reason is that the verbs are defined statically in the code so by the time the atom is assigned they are already defined using the default language. (here for example)

The original gettext read the dictionary from a configuration file so it worked statically; we'll need to think this one a bit more to make it work.

I think for the purposes of your game you'll be better off by forking and changing the default dictionary to point to the namespace with your translations.

@facundoolano
Copy link
Owner

Another option to explore is to use closure-defines to point a translation dictionary in project.clj

That would work for clojurescript (which is the priority), and maybe we could bring back carica for clojure.

@CharlesHD
Copy link
Contributor Author

Oh I see ! indeed verbs are statically constructed.

  • A simple workaround is to wrap that construction in a function (and do the same thing for the default verb-map construction) so it will be dynamically construct at the game creation and will fetch the translations. I don't think it will add that much start up time for the game right ?
  • Another workaround is to define the dictionary staticaly just like gettext and bind it at namespace evaluation.

Isn't there an issue on how the dictionary is defined and how your gettext function work ?
If I look here I see that the es dictionary key for "look around" is in a regex form : "^look around$".

But If I look at the gettext function I see there is no mecanism to match that regex. So when a call to (gettext "look around") is made my guess is it won't hit the right key in the dictionary map. I tested it on the REPL and indeed it won't:

example.game> (advenjure.gettext.core/gettext "look around")
"look around"
example.game> (advenjure.gettext.core/gettext "^look around$")
"^mirar alrededor$"

Here I see two way of doing it:

  • On one hand we could redefine the es dictionary to match the way verbs command are named (I think it's the easy solution).
  • On the other hand we could redefine gettext to match more complex construct like the ones in the es dictionary.

@CharlesHD
Copy link
Contributor Author

CharlesHD commented Oct 20, 2017

I just wrote two commits to my branch. The first one explore the dynamically construct verbs and verb-map. The second rewrite the espagnol dictionary for matching the way you refer to text in the verbs file.
I also add a commit to my advenjure example fork that test the es dict.

Es verbs are indeed available. The dictionary isn't full yet, and some verbs aren't dynamically defined either (tike take). I'll fix that this afternoon.

Edit : there is not just verbs that are statically constructed, but directions too. Directions are a little more deeply coupled with the rest of the code. I'll put that on my stack too but I think it needs more reflexion.

@CharlesHD
Copy link
Contributor Author

CharlesHD commented Oct 20, 2017

OK. After some reflexion I just hit the conclusion that using static dictionary would be far more easier. (Sorry for experimenting, you have a far larger overview of your project than I do)

I just saw your original gettext library and I think it's pretty neat and that we should make it work for clojurescript. I guess the missing point was carica not portable to clojurescript ? I suggest we use cprop that is a both java / javascript configuration reader.

EDIT : I'm wrong, cprop isn't clojurescript compliant. Searching for something working…

@CharlesHD
Copy link
Contributor Author

Okay, was wondering how people are doing to compile clojurescript code using configurations maps.

The trick is to bind the configuration at compile time using macro and it work as is in clojurescript. An exemple is given in this discussion.

If we just wrap your carica calls in a macro I think gettext should be portable as is.

@facundoolano
Copy link
Owner

Did you see my comment above about closure-defines? I think that should be enough for clojurescript.

@CharlesHD
Copy link
Contributor Author

CharlesHD commented Oct 20, 2017

Yes it's working too. I just supposed you would like having your previous workflow working.

I made another fork here where I show how it could be done. This solution works for both clojure and clojurescript and should be use in gettext. The key is to load the dictionary during macro expand time.

@facundoolano
Copy link
Owner

Nice, that looks good. You can make A PR with that change and let me test it over the weekend.

@facundoolano
Copy link
Owner

closing this in favor of #43

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