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

feat: added options object with parse option #21

Conversation

SvetozarMateev
Copy link
Contributor

Changes

This PR implements:

  • Bugfix
  • Feature
  • Refactoring
  • Documentation/wiki/tutorial update
  • Code style update
  • Build related change
  • Other, please specify:

Details

What does this PR implements exactly?
This is a feature that I think would be useful because there is no other library that can fix a string to be a valid JSON and with the options object and the parse property that will be possible.

@commit-lint
Copy link

commit-lint bot commented Oct 20, 2019

Contributors

@SvetozarMateev

@Berkmann18 Berkmann18 changed the title Added options object with parse option feat: added options object with parse option Oct 22, 2019
index.js Outdated
optionsCopy.parse = true;
}

console.log("opt", optionsCopy)
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this be something to print if options.verbose = true or was that just to test and not meant to stay there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot this during debugging ... I'll fix it.

index.js Outdated
* @param {boolean} [verbose=false] Verbosity
* @returns {{data: (Object|string|Array), changed: boolean}} Result
*/
const checkJson = (data, verbose = false) => {
const checkJson = (data, options, verbose = false) => {
Copy link
Owner

Choose a reason for hiding this comment

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

As options is there, do you think it would make more sense in merging verbose into it (ie. it will be a key in options like parse)?

That way, checkJson doesn't need an extraneous 3rd argument which could simply be specified in the second one (e.g. checkJson(..., {parse: ..., verbose: ...}).

Also, thank you for your contribution 😁.

Copy link
Owner

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Berkmann18
Copy link
Owner

@all-contributors Please add @SvetozarMateev for code

@allcontributors
Copy link
Contributor

@Berkmann18

I've put up a pull request to add @SvetozarMateev! 🎉

Berkmann18 pushed a commit that referenced this pull request Dec 15, 2019
# [1.4.0](v1.3.2...v1.4.0) (2019-12-15)

### Bug Fixes

* **fixer:** improved trailing character fixer ([d671f0c](d671f0c)), closes [#18](#18)
* **package-lock:** vulnerability fix ([a61b48a](a61b48a))
* **security:** bumped outdated packages + fixed vulnerabilities ([78bf487](78bf487))

### Features

* added options object with parse option ([#21](#21)) ([22874df](22874df))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants