CLI specifying elements via -e fails

kaicarver commented 8 years ago

Specifying elements via the command line parameter -e and -a does not work with multiple elements or attributes.

Documentation says default elements are title,p,h1,h2,h3,h4,h5,h6 but specifying them via -e seems to be like asking to localize no elements, returning strings only for the alt attribute, see output below.

It looks like the problem is with the comma separator, because specifying a single element or attribute works as expected.

$ s18n extract -help

  Usage: s18n-extract [options]

  Options:

    -h, --help                output usage information
    -a, --attributes [attrs]  html attributes to localize [default: "alt,title" | none: "."]
    -d, --directives [dirs]   html attributes to mark elements for localization [default: "localize" | none: "."]
    -e, --elements [elems]    html elements to localize [default: "title,p,h1,h2,h3,h4,h5,h6" | none: "."]
    -p, --paths [paths]       paths or patterns (globby) to find parsable html files [default: "**/*.html"]
    -s, --string <html>       extract from string of parsable html
    --hash <algo>             hashing algorithm to calculate string IDs [default: md5]
    --length <length>         length of string IDs [default: 8]
    --format <type>           format in which to return the locale, "object" or (JSON) "string" [default: "string"]

  Examples:

    $ s18n extract -s "<p>Hello, World!</p>"
    $ s18n extract -p "**/*.html, !notranslate/*.html"
    $ s18n extract -a . -e . -d translate=yes
    $ s18n extract > locales/en.json

kai@bluesteel ~/github/web
$ s18n extract -p index.html | head -7
{ '43334488': '<ul>\n<li><a href="http://yowureport.com/?p=6498">Yowureport - Hackerspaces & Makerspaces</a></li>\n<li><a href="/AssociationAnnouncement.jpg">Association announcement in 都會時報 (2014/08/01)</a></li>\n</ul>\n\n\n<div class="page-header" id="sponsors">\n  <h1>Supporters of the Taipei Hackerspace</h1>\n</div>',
  '48292469': '圓環(南京) (to East)',
  '66376152': 'Github Organization / Repository',
  '01af1289': '+886-2-2550-7630',
  c20ad4d7: '12',
  fc916446: '1386nJBU5xV5gdR9pMAea2wRSz6rz4apFu',
  b2eb7349: '306',

kai@bluesteel ~/github/web
$ s18n extract -e title,p,h1,h2,h3,h4,h5,h6 -p index.html
{ '5e717dc2': 'AskMorris',
  '7bc4a85e': 'CTimes',
  '36df9c5f': 'Eiger',
  '22845bef': 'Enspyre',
  '0511c146': 'Gandi',
  '6c8754a5': 'Hackerspace support Bitcoin QR code',
  c31010d1: 'Inhon',
  d2bc36f8: 'MobileGeeks',
  '282c3cc8': 'OSSLab',
  d794d3f7: 'Via' }

kai@bluesteel ~/github/web
$

For reference, HTML file is here: https://github.com/taipeihackerspace/web/blob/master/index.html

kaicarver commented 8 years ago

I boldly tried to fix the problem, but probably did it wrong: forked, branched, made correction, then was asked to install commitizen in order to be able to commit, ran cz, gave a lot of comment info, ... and then I'm not sure what happened, had to control-C out of cz, not very zen-like experience.

Anyway it was a one-line change in arrayify(), here it is:

lib/extract.js, line 187:
< return [unknown]
> return unknown.split(',');

bitjson commented 8 years ago

Hey @kaicarver – thanks for opening the issue. I'd love to accept a pull request if you want to take another swing at it! I'm betting the tests failed when you tried to commit. You can run the gulp task to watch and test as you're building, see: https://github.com/bitjson/s18n#contributing I'll also try to look into this a little later.

kaicarver commented 8 years ago

OK maybe I'll try again some time. But honestly I am beginning to wonder if this module, though it looks very nice, is usable for me, because I keep running into very basic stuff that doesn't work at all (I'll post one more issue). Maybe it's just the CLI that doesn't work well and that no one uses, and the library works well?

bitjson commented 8 years ago

Hey @kaicarver – this CLI is definitely lacking. By far the most used interface for s18n is gulp-l10n, if you'd prefer something more stable. While s18n's javascript API has 100% test coverage, the CLI is not tested by the build. The gulp plugin, however, has 100% test coverage over the entire interface, so you shouldn't encounter any issues over there.

That being said, I'm planning to take another pass of s18n's CLI in the next few days, so hopefully I can clean it up a bit, maybe even add the CLI to test coverage (#2) so it can't rot so easily.

Thanks for posting the issues you're finding!

kaicarver commented 8 years ago

thanks for the tip, I will try using s18n via gulp-l10n. My initial goal was to do a super-quick-and-dirty localization, so I'll try to keep doing that, and maybe make some bugfix or feature contributions later.

bitjson commented 8 years ago

Hey @kaicarver – is this solved now by #11? Thanks for the PR!

kaicarver commented 8 years ago

It should be! No time to make an automated test, but it did fix it on my local version from a few casual tests. On Apr 21, 2016 23:20, "Jason Dreyzehner" notifications@github.com wrote:

Hey @kaicarver https://github.com/kaicarver – is this solved now by #11 https://github.com/bitjson/s18n/pull/11? Thanks for the PR!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/bitjson/s18n/issues/8#issuecomment-212969293

bitjson commented 8 years ago

Great, thanks! I'll close this for now, and I'll try to write some good test cases soon.

bitjson / s18n

CLI specifying elements via -e fails #8