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

Adds parser option to allow for different storage formats #488

Merged
merged 6 commits into from May 19, 2022

Conversation

mathiashsteffensen
Copy link

Will close #487

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

@mashpie
Copy link
Owner

mashpie commented Jan 23, 2022

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
Copy link
Owner

mashpie commented Jan 23, 2022

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

@coveralls
Copy link

coveralls commented Jan 24, 2022

Coverage Status

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

@guyaumetremblay
Copy link

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

@mashpie
Copy link
Owner

mashpie commented Apr 29, 2022

yes

@mashpie
Copy link
Owner

mashpie commented Apr 29, 2022

...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
Copy link

@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
Copy link
Owner

mashpie commented Apr 29, 2022

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

@mashpie mashpie merged commit 7bddaec into mashpie:master May 19, 2022
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.

[Discussion] Allow a parser option to customize storage format
4 participants