search

mashpie / i18n-node

Lightweight simple translation module for node.js / express.js with dynamic json storage. Uses common __('...') syntax in app and templates.
MIT License
3.09k stars 421 forks source link

Adds parser option to allow for different storage formats #488

Closed mathiashsteffensen closed 2 years ago

mathiashsteffensen commented 2 years ago

Will close #487

This is my first time contributing to any 'real' OSS, so please don't hold back with your critique :)

mashpie commented 2 years ago

Thank you for your contribution and fast response. I'd say you hold on a little, as today I have some minor refactoring upstream which will be conflicting and require a rebase. :)

Your changes are well focused and readable. I only have some simple points to consider:

1) add YAML to devDependencies - this should fix pipeline too 2) README: Configure should outline defaults by default and give instructions on possible setting. It should mention to have require('YAML') 3) When writing tests, better use new I18n({}) (like in https://github.com/mashpie/i18n-node/blob/master/test/i18n.noTraversal.js) and bind your tests to a local object

Thanks again... I'd ping as soon I am done.

mashpie commented 2 years ago

Alright, @mathiashsteffensen - feel free to proceed...

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.003%) to 98.175% when pulling e55a597567ce5e6b9edbc23824ef7d487f2e62fd on mathiashsteffensen:custom-parser into 192086f212e14593316d04586d0c6e6c163d772a on mashpie:master.

guyaumetremblay commented 2 years ago

@mathiashsteffensen @mashpie Hi! I'm really interested by this PR. Do you plan to merge it soon?

mashpie commented 2 years ago

yes

mashpie commented 2 years ago

...apply updates, resolve conflicts, retest and review. (maybe complete testing) It's a bit more than "just merging". But Okay, I'll take some time and some coffee "soon".

guyaumetremblay commented 2 years ago

@mashpie I was asking as the PR is 3 months old and the initiator start to update the PR also 3 months ago. So, I was asking if the work to merge it was still in progress or on hold... Sorry if my request looked like me asking to do this "ASAP".

mashpie commented 2 years ago

All good. No worries :) it‘s More like: Every task Taking more than some minutes will be lost… These days. So: Thanks for reminding.