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

CSV formatter #188

Closed
wants to merge 4 commits into from
Closed

Conversation

Bogidon
Copy link
Contributor

@Bogidon Bogidon commented Mar 23, 2017

Yet Another Try at adding a CSV formatter (related: #34, #41, #154).

I went for a barebones formatter, opting out of creating a convention for encoding multiple languages in one CSV file. The format is similar to Apple's .strings except that language information is encoded in the file name:

  • strings-en.csv
  • strings-ro.csv
  • etc.

Here's some sample CSV, pulled out from the test fixtures:

Key,Value,Comment
key1,value1-english,comment key1
key2,value2-english,
key3,value3-english,
key4,value4-english,comment key4

I was a little confused about where exactly I should be adding tests. Just let me know what I should fix and I'll jump on it!

The CSV formatter can only handle one language per file. Fields are formatted as `key,value,comment`.
Previously forced quotes so `strip` could be run on each section to remove newlines. Changed from `strip` to a RegExp that removes only lines that are completely empty. Quotes no longer mandatory.
@sebastianludwig
Copy link
Collaborator

Thank you very much for your work! At a first glance it looks very good, I'll take a closer look on the weekend 👍

@Bogidon
Copy link
Contributor Author

Bogidon commented Mar 24, 2017

Great, thanks!

@scelis
Copy link
Owner

scelis commented Mar 24, 2017

Interesting. Thank you for this! I am curious, though. I would imagine that people using CSV would want the file to contain multiple translations instead of just one. Is there anyone out there with a use-case for CSV files that only have a single translation?

Thus, how useful is this current implementation? And if we decide to add multiple languages later, would that break anyone who decided to start using this one?

@Bogidon
Copy link
Contributor Author

Bogidon commented Mar 24, 2017

@scelis, yeah I'm starting to lean towards multi-language CSVs too. I went for single language because it is I need in my particular use case, but I can easily see how multi-language would make a lot more sense. Just to make sure we consider all options, two other solutions could be

  1. a separate multi-language CSV formatter
  2. a flag that toggles between multi-language and single-language. (though this is probably undue complexity and probably an anitpattern)

I'll update the PR for whatever we decide on.

@scelis
Copy link
Owner

scelis commented Apr 4, 2017

I don't think we need an extra flag or an extra formatter. What do you think about using the existing --lang flag? That flag should support a comma-separated to support multiple languages. That way the single formatter could be used to create a single language CSV or a multi-language one. The only update this PR would need would be to add a header row describing the position of each language in the file. For example:

key,en,es,comment
hello,Hello,Hola,A greeting

Copy link
Collaborator

@sebastianludwig sebastianludwig left a comment

Choose a reason for hiding this comment

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

Short and concise 👍 I only added two comments/questions.


def format_section(section, lang)
# removes empty lines
super.gsub(/^$\n/, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's important to not have empty lines in the CSV, we should add a unit test for this


def read(io, lang)
while line = io.gets
fields = ::CSV.parse_line(line)
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this also add the header as translation?

@sebastianludwig
Copy link
Collaborator

In general I like the idea of having a CSV formatter a lot. It's been requested multiple times. However I also think it should be a multi-language formatter. By default it should output all languages if the --lang parameter is omitted.

@Bogidon do you want to give the implementation a go?

(Sorry again for taking ages to react :-/)

@Bogidon
Copy link
Contributor Author

Bogidon commented Apr 20, 2017

Also sorry for leaving this up in the air for so long (my I18N project at worked got temporarily shelved). I'll definitely have a go on it, hopefully in the next few days.

@scelis
Copy link
Owner

scelis commented Apr 9, 2019

Closing due to inactivity. Please reopen if you ever have time to revisit!

@scelis scelis closed this Apr 9, 2019
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.

None yet

3 participants