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

changedRows is English language-specific #1819

Closed
jniles opened this issue Sep 13, 2017 · 7 comments
Closed

changedRows is English language-specific #1819

jniles opened this issue Sep 13, 2017 · 7 comments
Assignees
Labels

Comments

@jniles
Copy link
Contributor

jniles commented Sep 13, 2017

The changeRows property is set from a regex that assumes an English language message response. This functionality breaks in multi language environments. For example, a French installation of MySQL will return modifié, which will be not be caught by the regex.

In my view, there are two solutions:

  1. Attempt to improve the regex to be multi-language.
  2. Document the language-specific nature of this property and suggest that non-English environments use affectedRows instead.

For further relevant information, see the discussion in #251 which pointed me to the changedRows property.

jniles added a commit to jniles/mysql that referenced this issue Sep 13, 2017
This commit documents that `changedRows` is dependent on parsing the
server message and is English-specific.

Closes mysqljs#1819.
jniles added a commit to jniles/mysql that referenced this issue Sep 13, 2017
This commit documents that `changedRows` is dependent on parsing the
server message and is English-specific.

Closes mysqljs#1819.
@dougwilson
Copy link
Member

Ideally if it can be modified to support all languages that would be much better than a doc change. Docs can be easily overlooked and of course just working right is better anyway :)

@jniles
Copy link
Contributor Author

jniles commented Sep 13, 2017

That sounds like the best solution. In light of #1819 (comment), would you like me to close #1820?

@dougwilson dougwilson added the bug label Sep 24, 2017
@dougwilson
Copy link
Member

I can still merge the docs change, and keep this open for tracking getting it to actually work for all the languages.

@dougwilson
Copy link
Member

Here is the translation strings from 5.7.17 for the message:

ER_UPDATE_INFO  
        cze "Nalezených řádků: %ld  Změněno: %ld  Varování: %ld"
        dan "Poster fundet: %ld  Ændret: %ld  Advarsler: %ld"
        nla "Passende rijen: %ld  Gewijzigd: %ld  Waarschuwingen: %ld"
        eng "Rows matched: %ld  Changed: %ld  Warnings: %ld"
        est "Sobinud kirjeid: %ld  Muudetud: %ld  Hoiatusi: %ld"
        fre "Enregistrements correspondants: %ld  Modifiés: %ld  Warnings: %ld"
        ger "Datensätze gefunden: %ld  Geändert: %ld  Warnungen: %ld"
        hun "Megegyezo sorok szama: %ld  Valtozott: %ld  Warnings: %ld"
        ita "Rows riconosciute: %ld  Cambiate: %ld  Warnings: %ld"
        jpn "該当した行: %ld  変更: %ld  警告: %ld"
        kor "일치하는 Rows : %ld개 변경됨: %ld개  경고: %ld개"
        por "Linhas que combinaram: %ld - Alteradas: %ld - Avisos: %ld"
        rum "Linii identificate (matched): %ld  Schimbate: %ld  Atentionari (warnings): %ld"
        rus "Совпало записей: %ld  Изменено: %ld  Предупреждений: %ld"
        serbian "Odgovarajućih slogova: %ld  Promenjeno: %ld  Upozorenja: %ld"
        spa "Líneas correspondientes: %ld  Cambiadas: %ld  Avisos: %ld"
        swe "Rader: %ld  Uppdaterade: %ld  Varningar: %ld"
        ukr "Записів відповідає: %ld  Змінено: %ld  Застережень: %ld"

@dougwilson
Copy link
Member

dougwilson commented Sep 24, 2017

It looks like it's the only message that would match the language-neutral pattern /^[^:0-9]+: ([0-9]+)[^:0-9]+: ([0-9]+)[^:0-9]+: ([0-9]+)[^:0-9]*$/

@dougwilson dougwilson self-assigned this Sep 24, 2017
@dougwilson
Copy link
Member

Ok, so looks like that regular expression seems to work fine. If you want to try it out before the next release, you can use the instructions at https://github.com/mysqljs/mysql#install to install the master branch to test.

@jniles
Copy link
Contributor Author

jniles commented Sep 25, 2017

Awesome! I'll give it a whirl and comment back when we have some results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants