scrapinghub / js2xml

Convert Javascript code to an XML document
MIT License
186 stars 23 forks source link

Switch from slimit to calmjs.parse #33

Closed Gallaecio closed 4 years ago

Gallaecio commented 5 years ago

Fixes #31

Notes:

codecov[bot] commented 5 years ago

Codecov Report

Merging #33 into master will decrease coverage by 0.81%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   90.12%   89.30%   -0.82%     
==========================================
  Files          11        6       -5     
  Lines         749      533     -216     
==========================================
- Hits          675      476     -199     
+ Misses         74       57      -17     
Impacted Files Coverage Δ
js2xml/__init__.py 100.00% <100.00%> (ø)
js2xml/xmlvisitor.py 97.04% <100.00%> (+0.27%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8b44247...1559c3d. Read the comment docs.

metatoaster commented 5 years ago
  • I’ve removed our custom Lexer and Parser, and with them the logging module. Is that OK?

  • I’ve removed lextab.py (autogenerated?) and yacctab.py, as they did not seem necessary anymore. I’m not completely sure, though.

As far as I can tell, what was there previously that this patch replaced was to work around the issue with slimit causing those warnings about the *tab modules, and the custom *tab.py files included here were to ensure that they worked with the latest ply package available at the time (which is not the case anymore, as ply is now at 3.11 while the one shipped currently with js2xml is one release behind at 3.10). Given that one of my objectives of creating calmjs.parse was to make these workarounds unnecessary for dependent packages, they should be safe to remove.

More details on that: the wheels I intend to ship with calmjs.parse will contain those *tab modules for all supported versions of ply as they are useful optimisation modules (i.e. new calmjs.parse release when new ply release necessitates *tab regeneration), and thus this would eliminate the need for dependent/downstream packages (such as this one) to micromanage that. I also intend to maintain backwards compatibility/API stability given the stability of historical ECMAScript specs, such that it should be safe to depend on any future versions of calmjs.parse (so that future version(s) of ply will also be supported simply by upgrading; at least this is the plan, as it does hinge on future ply versions not breaking compatibility in a way that breaks calmjs.parse).

Thank you for creating this patch and for making use of calmjs.parse. Also fascinating for me to see that the changes that made use of the "visitor" pattern still continue to work.