Skip to content

feat: add utils/parse-ndjson #1394

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 67 commits into from
Mar 8, 2024
Merged

feat: add utils/parse-ndjson #1394

merged 67 commits into from
Mar 8, 2024

Conversation

maniksharma17
Copy link
Contributor

Resolves #1075 .

Description

adding the package @stdlib/utils/parse-ndjson

The package is similar in structure as [@stdlib/utils/parse-json]
(lib/node_modules/@stdlib/utils/parse-json)

Package: @stdlib/utils/parse-ndjson
Alias: parseNDJSON

What is the purpose of this pull request?

This pull request:

  • adds the package @stdlib/utils/parse-ndjson

Related Issues

Does this pull request have any related issues?
No.
This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

stdlib-bot
stdlib-bot reviewed Feb 26, 2024
View reviewed changes
Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@maniksharma17 maniksharma17 changed the title feat: added @stdlib/utils/parse-ndjson #1075 feat: added @stdlib/utils/parse-ndjson Feb 26, 2024
@maniksharma17 maniksharma17 changed the title feat: added @stdlib/utils/parse-ndjson feat: add @stdlib/utils/parse-ndjson Feb 26, 2024
@Planeshifter Planeshifter self-requested a review February 27, 2024 01:11
@Planeshifter
Copy link
Member

/stdlib update-copyright-years

@Planeshifter
Copy link
Member

@maniksharma17 Thanks for your contribution! Overall it's coming along well and should be ready to merge soon; there are a bunch of lint errors currently (see CI); I will do a proper review tomorrow.

@maniksharma17
Copy link
Contributor Author

@Planeshifter
Thanks for reviewing, is there anything to change or resolve on my part?

@maniksharma17
Update README.md
5286d31 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
@maniksharma17
Copy link
Contributor Author

@Planeshifter hey, I updated the README to correct some errors.

@maniksharma17 maniksharma17 mentioned this pull request Feb 27, 2024
Closed
1 task
@maniksharma17
Update benchmark.js
4b3d5c3 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
@shubhexists
Copy link
Contributor

Reply to - #1399 (comment)

@maniksharma17 I ran the benchmarks after setting up your branch on my system.
It worked fine for me. Maybe you can try reinstalling your node-modules....

Screenshot from 2024-02-28 01-08-05

@maniksharma17
Copy link
Contributor Author

@shubhexists Thanks for the help!

@kgryte kgryte changed the title feat: add @stdlib/utils/parse-ndjson feat: add utils/parse-ndjson Feb 28, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. Utilities Issue or pull request concerning general utilities. labels Feb 28, 2024
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
lib/node_modules/@stdlib/utils/parse-ndjson/benchmark/benchmark.js Outdated
Comment on lines 42 to 46
for ( let i = 0; i < out.length; i++ ){
if ( out[i] instanceof Error ){
b.fail( 'should return an array of JSON objects' );
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't include this much of an assertion within the code being measured. Prefer lightweight checks. See @stdlib/bench/harness for benchmark guidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte reviewed Feb 28, 2024
View reviewed changes
kgryte
kgryte requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@maniksharma17 Thanks for working on this. At the moment, this contribution requires many changes. Please see other packages and try to match the style and conventions of those packages as closely as possible. E.g., ES5, documentation, spacing, etc. Until this package has been updated accordingly, we won't be able to merge.

@maniksharma17
Update README.md
76c6be0 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
@maniksharma17
Copy link
Contributor Author

Could you approve for workflows, and review it?
When this merges, I could contribute more to stdlib without the hassle of waiting for workflow approvals.

@Planeshifter @kgryte

@maniksharma17
Update index.js
8f01e53 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
Planeshifter
Planeshifter requested changes Mar 8, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

@maniksharma17 Looks like there are still some lint errors. Make sure that you have your development environment properly initialized (make init) and I also recommend installing an ESLint extension if you are using VSCode or Sublime Text, which should highlight lint errors directly in your editor. You can also run linting via the make lint commands.

@maniksharma17
Update main.js
2064f36 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
@maniksharma17
Update test.js
1f76d20 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
@Planeshifter
Apply suggestions from code review
8af4a02 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update lib/node_modules/@stdlib/utils/parse-ndjson/docs/types/test.ts
eb2b06b 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update lib/node_modules/@stdlib/utils/parse-ndjson/lib/main.js
f128dba 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update lib/node_modules/@stdlib/utils/parse-ndjson/lib/main.js
3efc890 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
@Planeshifter
Update lib/node_modules/@stdlib/utils/parse-ndjson/docs/repl.txt
21e58d9 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Planeshifter
Planeshifter reviewed Mar 8, 2024
View reviewed changes
Planeshifter and others added 4 commits March 8, 2024 09:42
@Planeshifter
Update lib/node_modules/@stdlib/utils/parse-ndjson/examples/index.js
c112f7f 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update index.js
1fc6a35 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update test.js
ea33c68 
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@maniksharma17
Update benchmark.js
00af9d3 
Signed-off-by: Manik Sharma <maniksharma.rke@gmail.com>
Planeshifter
Planeshifter approved these changes Mar 8, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thanks @maniksharma17! Looks good now, will merge your contribution after CI passes! 🚀

@Planeshifter Planeshifter merged commit 8d6f33c into stdlib-js:develop Mar 8, 2024
@maniksharma17
Copy link
Contributor Author

@Planeshifter @kgryte
Hey, thank you for the support 🤝
I'll take care of lint errors next time.
This was my first ever open-source contribution.

@maniksharma17 maniksharma17 deleted the feat/1075-added-parse-ndjson branch March 9, 2024 17:08
@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Planeshifter Planeshifter Planeshifter approved these changes

@stdlib-bot stdlib-bot Awaiting requested review from stdlib-bot

@kgryte kgryte Awaiting requested review from kgryte

Assignees
No one assigned
Labels
Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/utils/parse-ndjson
5 participants
@maniksharma17 @Planeshifter @shubhexists @kgryte @stdlib-bot