Workbook.DefinedName(s) support has been added. #161

2015-01-15T08:53:44Z

alitskevich commented

2015-01-15 08:53:44 +00:00

(Migrated from github.com)

From this fork https://github.com/alitskevich/js-xlsx

coveralls commented

2015-01-15 09:00:11 +00:00

(Migrated from github.com)

Coverage remained the same when pulling 83f425368a on alitskevich:master into 61b17e6d9d on SheetJS:master.

[![Coverage Status](https://coveralls.io/builds/1746544/badge)](https://coveralls.io/builds/1746544) Coverage remained the same when pulling **83f425368a77d9209c985e5f7c70ae8b7b52720e on alitskevich:master** into **61b17e6d9d2544304cff2a57bdac09225360d660 on SheetJS:master**.

SheetJSDev commented

2015-01-15 18:06:44 +00:00

(Migrated from github.com)

Generally looks good, not in front of computer at the moment so I can't test just yet. Few comments:

can you lift the regex definitions out of the function? if you look just above your change, wbnsregex is lifted out of parse_wb_xml so that it's processed at load time
can you squash down to one commit?
does make lint pass?

Generally looks good, not in front of computer at the moment so I can't test just yet. Few comments: 1) can you lift the regex definitions out of the function? if you look just above your change, wbnsregex is lifted out of parse_wb_xml so that it's processed at load time 2) can you squash down to one commit? 3) does `make lint` pass?

alitskevich commented

2015-01-15 20:28:40 +00:00

(Migrated from github.com)

Thank you for fast feedback (it is my very first PR on github),

not sure, because call re.exec() for //g modifying re.lastIndex.
2-3) ok, i'll check with lint and prepare a new PR tomorrow
seems it also needs to add utf8read(attrValue)

Thank you for fast feedback (it is my very first PR on github), 1) not sure, because call re.exec() for //g modifying re.lastIndex. 2-3) ok, i'll check with lint and prepare a new PR tomorrow 4) seems it also needs to add utf8read(attrValue)