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

i18n #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

i18n #1

wants to merge 1 commit into from

Conversation

alanmtk
Copy link
Owner

@alanmtk alanmtk commented May 9, 2019

Implemented node-i18n and added es/en locales

@josx
Copy link

josx commented May 14, 2019

Varias cosas para remarcar:

  • Primero que nada esta muy bien usar la lib i18n que es muy simple y parece ser bastante liviana

  • El segundo commit no debería existir ya que es algo que deberia estar bien hecho en primer momento, ahi deberias hacer un squash de ese commit.

  • El repo tiene el build de la aplicación commiteado, obviamente no es una buena práctica. Deberiamos igualmente pensar que hacer con esto (si preguntar al programador, mandarle el build o como bien hiciste no ponerlo)

  • No me funciona:

josx@idilia /tmp/node-games⚡git:i18n* $ DEBUG=i18n:* node index.js
  i18n:debug will use /tmp/node-games/locales/en.json +0ms
  i18n:debug read /tmp/node-games/locales/en.json for locale: en +3ms
  i18n:debug will use /tmp/node-games/locales/es.json +0ms
  i18n:debug read /tmp/node-games/locales/es.json for locale: es +0ms
usage: node-games <game>

  i18n:warn WARN: No locale found - check the context of the call to __(). Using en as current locale +2ms
Games
- spacecraft
- snake
- tanks
josx@idilia /tmp/node-games⚡git:i18n* $ locale
LANG=es_AR.UTF-8
LANGUAGE=es_AR:es
LC_CTYPE="es_AR.UTF-8"
LC_NUMERIC="es_AR.UTF-8"
LC_TIME="es_AR.UTF-8"
LC_COLLATE="es_AR.UTF-8"
LC_MONETARY="es_AR.UTF-8"
LC_MESSAGES="es_AR.UTF-8"
LC_PAPER="es_AR.UTF-8"
LC_NAME="es_AR.UTF-8"
LC_ADDRESS="es_AR.UTF-8"
LC_TELEPHONE="es_AR.UTF-8"
LC_MEASUREMENT="es_AR.UTF-8"
LC_IDENTIFICATION="es_AR.UTF-8"
LC_ALL=

src/tanks.js Outdated
@@ -1,6 +1,7 @@
import Unit from './classes/unit';
import Tank from './classes/tank';
import Interface from './classes/interface';
import i18n from "i18n";
Copy link

Choose a reason for hiding this comment

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

Si en todos lados estan usando single quote deberias usar single quote, no double quote.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Opté por remover ese import, ya no era necesario porque es importado en index.js

Copy link

Choose a reason for hiding this comment

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

genial

index.js Outdated
i18n = require('i18n');

i18n.configure({
directory: __dirname + '/locales'
Copy link

Choose a reason for hiding this comment

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

¿Esto funciona en windows? o habría que por las dudas usar
path.join o similar.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Lo cambié por por path.join(__dirname, '/locales') que nos saca el problema de los slashes según sistema operativo

Copy link

Choose a reason for hiding this comment

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

perfecto

@alanmtk
Copy link
Owner Author

alanmtk commented May 14, 2019

@josx No te esta funcionando porque no están las cosas buildeadas como originalmente vienen en el repo, en un principio no lo subí por una cuestión de que no es buena practica pero si es cierto que sin eso y siguiendo las instrucciones del README no funcionaría si lo descargan. En cuanto a lo de los multiples commits desde la pull puedo hacer un "Squash and merge" que junta todos los commits y mergea, es esa la idea o debería encararlo de otro modo?

@josx
Copy link

josx commented May 16, 2019

  • Todos los que son Arreglos ej: fix deberias usar squash o fixup, y tener separados los que creas que son claramente algo en si mismo pertinente de marcar. (luego podemos charlar sobre las diferencias y como se hace esto úlitmo sin github con in git rebase interactivo)

  • Sobre el funcionamiento ¿Como lo estas probando vos?

@alanmtk alanmtk force-pushed the i18n branch 2 times, most recently from 22077d9 to 4c9abab Compare May 16, 2019 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants