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

[WIP] added csv to json for javascript #4

Closed
wants to merge 6 commits into from
Closed

[WIP] added csv to json for javascript #4

wants to merge 6 commits into from

Conversation

veeralsharma
Copy link

added functionality for converting csv to json , just added the location of your file in path and you are good to go

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

Hey, @veeralsharma Thank you very much for your contributions. I'm glad you invested time in making the project better. The script runs fine, and there are few more changes we can work on before I merge this.

Output format

The current output of the script is

[{"vanilla-i18n-key":"language","English":"Language","हिन्दी":"भाषा","española":"idioma","français\r":"Langue\r"},{"vanilla-i18n-key":"form.name","English":"Name","हिन्दी":"नाम","española":"nombre","français\r":"Nom\r"},{"vanilla-i18n-key":"form.age","English":"Age","हिन्दी":"उम्र","española":"años","français\r":"âge\r"},{"vanilla-i18n-key":"form.resp.yes","English":"Yes","हिन्दी":"हाँ","española":"si","français\r":"Oui"}]

However, the desired output format is in a file, one file for each language, in a folder vanilla-i18n. For example, the english.json and français.json after running the script on example.csv should have the following:

{"language":"Language","form":{"name":"Name","age":"Age","resp":{"yes":"Yes"}}}
{"language":"Langue","form":{"name":"Nom","age":"\u00e2ge","resp":{"yes":"Oui"}}}

Readme Updates

Since you are adding a new script, you may also want to update the README.md inside csv_to_vanilla_i18n folder to include information about your created script - take a look at existing information about the python script for inspiration.

Other Minor Changes

I've commented on the lines/sections of code where I had to provide suggestions, please see if you can consider them 💯

Let's together make the PR better and then merge it, it would be an ama

csv_to_vanilla-i18n/csv_to_json.js Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/csv_to_json.js Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/csv_to_json.js Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/csv_to_json.js Outdated Show resolved Hide resolved
@veeralsharma
Copy link
Author

veeralsharma commented Oct 3, 2020 via email

@thealphadollar
Copy link
Owner

Hey Veeral, I'm afraid I cannot merge the PR in the current state since the script would mislead users. I totally understand and feel free to contribute as you are comfortable.

I'm making this a draft PR for the time being 😄

@thealphadollar thealphadollar changed the title added csv to json for javascript [WIP] added csv to json for javascript Oct 3, 2020
@thealphadollar thealphadollar marked this pull request as draft October 3, 2020 18:40
@veeralsharma
Copy link
Author

veeralsharma commented Oct 3, 2020 via email

@veeralsharma veeralsharma marked this pull request as ready for review October 3, 2020 21:44
@veeralsharma
Copy link
Author

dont put it in draft it kinda affect the whole hacktober fest thing , if possible merge it I don't think so it will mislead anyone now

@thealphadollar
Copy link
Owner

thealphadollar commented Oct 4, 2020

Hey @veeralsharma , I understand that you want to finish the hacktoberfest 4 PRs. However, I'm afraid I cannot accept a script that cannot be used - it'll mislead people to think that we have a node.js script and when they'll use it, the output will be different from what we accept and can be used with vanilla-i18n.

I understand your skills, and if you have written the script, I believe you can modify it to match the output format and structure so that I can quickly accept it. If completed this will be really useful for web developers :D

PS. A small note of humble request, please do not convert to "ready for review" if a maintainer marks it as "draft". It might come out as rude and spam 😄

@thealphadollar
Copy link
Owner

@veeralsharma Please let me know if you'd be continuing work on the PR - although the feature has been added since you have worked on it, I'll accept the PR if we can incorporate the requested changes.

@veeralsharma
Copy link
Author

veeralsharma commented Oct 11, 2020 via email

@veeralsharma
Copy link
Author

Hey everything is done from my side
I have incorporated all the changes you asked

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

@veeralsharma Hey thanks for the update. Can you please confirm that the output on running the script on example.csv is same as available in the existing folder vanilla-i18n in the script folder

@veeralsharma
Copy link
Author

I have corrected some things , and tested out completely and now it runs exactly how it is supposed to , you can check

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

Hey @veeralsharma Since this is a copy of https://github.com/thealphadollar/vanilla-i18n/blob/master/csv_to_vanilla-i18n/csv_to_vanilla_i18.js, I'm marking it as hacktoberfest-invalid. It made me really sad that you chose to duplicate the file at the end instead of improving your submission.

@veeralsharma
Copy link
Author

veeralsharma commented Oct 12, 2020 via email

@thealphadollar
Copy link
Owner

That's a good attitude @veeralsharma . All the best for future contributions ✨

@veeralsharma
Copy link
Author

veeralsharma commented Oct 12, 2020 via email

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