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

0-index null #32

Closed
Sinewyk opened this issue Oct 23, 2014 · 6 comments
Closed

0-index null #32

Sinewyk opened this issue Oct 23, 2014 · 6 comments

Comments

@Sinewyk
Copy link
Contributor

Sinewyk commented Oct 23, 2014

Hi, this has been bugging me for a while

// Note: by convention, the 0-index location of the translations
// is never accessed. It's just a thing, I guess.

what does this mean, convention by whom ? I look at my po files and all of them have all useful data.

They don't have any useless translation that is never accessed, so where is this convention coming from.
For kicks I've been going through the source code and deleting all ' + 1' there and there, and with an ugly

val_list = _.compact(val_list);

It just works, but there's no waste of an empty index in an array. Why would I parse an mo file to generate empty data ?

@SlexAxton
Copy link
Member

It's been ages since I wrote that, so I really don't know. At the time, I did a considerable chunk of research on the formats, so I doubt it was nothing. My gut is that po indexes are 1-indexed and arrays are 0-indexed, and we could just do the conversion in the math, rather than adding the blank index.

What's your best suggestion that you think might not break everyone's existing code?

@Sinewyk
Copy link
Contributor Author

Sinewyk commented Oct 23, 2014

I don't even know if it's needed, it's just been bothering me. But if it were done ... it should go hand in hand with and edit of that line I think
https://github.com/WrinklyNinja/jed-gettext-parser/blob/master/jedGettextParser.js#L174
and then people if they want could upgrade both packages.

But really, it's not NEEDed. But, sending across the wire useless info has been always bugged me 😆

edit: https://github.com/Sinewyk/ff-kit/blob/master/lang/fr.po#L56 seems to me that it's 0 indexed, so I don't know. This was generated by the lastest Poedit software.

@SlexAxton
Copy link
Member

I guess I am too far removed from these details to make a good call. If we just killed the + 1 lines in jed, and then had @WrinklyNinja kill the [null].concat then you think things would Just Work™ when people updated?

@Sinewyk
Copy link
Contributor Author

Sinewyk commented Oct 24, 2014

Pass both suites of test with Sinewyk/Jed@8cc37367 and Sinewyk/jed-gettext-parser@024bded1.

The last test of jed-gettext-parser did not work by npm test-ing (automatic).

I whipped up a python -m SimpleHTTPServer 8000 quickly to debug, expecting it to also fail, and it worked giving me the correct translation ... so, dunno if that's entirely valid.

Fact is, both suites are green one way or another. And I don't know what to do with this 😞 .

@Ortham
Copy link
Contributor

Ortham commented Oct 24, 2014

I'm not particularly bothered about the indexing, but if the null 0-index is removed, I'll happily merge a PR from @Sinewyk.

As for the jed-gettext-parser tests failing, is that not being cause the tests check for a null 0-index? I don't think that's been updated.

@Sinewyk
Copy link
Contributor Author

Sinewyk commented Oct 24, 2014

Wrong branch @WrinklyNinja ^^, use my link for the correct one + the commit ^^. But no, the test fail if using npm test, and the test works if I manually ln -s stuff, then launch a quick http server inside test/browser. The i18n.gettext('Manage Comments') is equal to 'Manage Comments' by npm test, and is correctly equal to ... its Russian equivalent in the browser I use myself.

I took that as a success for a quick POC.

@Sinewyk Sinewyk closed this as completed Nov 5, 2014
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

No branches or pull requests

3 participants