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: use JSON5 for INPUT.json #1538

Merged
merged 4 commits into from
Sep 14, 2022
Merged

feat: use JSON5 for INPUT.json #1538

merged 4 commits into from
Sep 14, 2022

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Sep 13, 2022

Fixes #905

@vladfrangu
Copy link
Member

Could add a test in packages/core/test/user-input.test.ts for inputs with comments 👀

@B4nan
Copy link
Member

B4nan commented Sep 13, 2022

We should probably move this to some other place. Right now you adjusted this only for crawlee (Actor.getInput() does not go thru this path) and only for the root ./INPUT.json file.

I would be in favour of doing this in the memory storage too, thoughts @vladfrangu?

edit: for input only I guess, or maybe for KVS, definitely not for dataset and RQ

@vladfrangu
Copy link
Member

If we want to add full JSON5 support, we should probably use https://github.com/json5/json5 or similar then. Could see it being useful for others dealing with JSON files, and I'd be fine with adding support for it in memory-storage

@B4nan
Copy link
Member

B4nan commented Sep 13, 2022

Can you please verify how does it stand performance wise?

I guess I would be up for it, just checked the size of crawlee package, and I guess there is no sense is trying to spare 200 kilobytes when its so huge :D

@pocesar
Copy link
Contributor

pocesar commented Sep 14, 2022

I'd vote for using the json5 battled-tested lib. it's still a pain to have multiple JSON files under default KV when testing locally.
image

@szmarczak szmarczak changed the title feat: accept JSON comments for INPUT.json feat: use JSON5 for INPUT.json Sep 14, 2022
@szmarczak
Copy link
Contributor Author

Added an E2E test for INPUT file loading. It also tests Actor.getInput().

@B4nan B4nan merged commit 09133ff into master Sep 14, 2022
@B4nan B4nan deleted the input-comments branch September 14, 2022 18:58
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.

Use JSON5 when parsing INPUT.json
4 participants