adhearsion / ruby_speech

A ruby library for TTS & ASR document preparation
MIT License
101 stars 41 forks source link

[BUGFIX] Ensure that Rulerefs n-levels deep are expanded #34

Closed sfgeorge closed 7 years ago

sfgeorge commented 9 years ago

When grammars are interpreted inline by RubySpeech itself, the intention was to expand all internal rulerefs to other, literal elements.

The problem: If a nesting exists in which a referenced rule references yet another rule, the expansion would be incomplete and inline matching would fail.

This fixes the issue by ensuring that rulerefs continue to be expanded until no more exist.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.04%) when pulling 99565490f5fa35cafbaeaa9e9c8735f8a3dd1bbf on sfgeorge:feature/ruleref-nesting into 303f90fcbb87dd235fb3a49ffa0c197cf8451eac on benlangfeld:develop.

sfgeorge commented 9 years ago

Cc: @Jared-Prime, @runningferret

sfgeorge commented 9 years ago

@benlangfeld Let me know if you'd like me to follow The Hound's advice thoroughly. Note though that there's much a' barking over things I simply changed the indentation to.

bklang commented 9 years ago

@benlangfeld Let me know if you'd like me to follow The Hound's advice thoroughly. Note though that there's much a' barking over things I simply changed the indentation to.

@sfgeorge It won't be a blocker to merge, especially for the parts you didn't actually change. If you can satisfy Hound on the parts you are contributing, that would be very helpful. Thanks for the contribution!

sfgeorge commented 9 years ago

Note: Added finite recursing limit (default: 25 iterations) to prevent an infinite loop when expanding rules in invalid grammars.

It should be noted from the GrXML spec it is not required that a grammar processor support recursive grammars:

A Conforming XML Form Grammar Processor is not required to support recursive grammars, that is, grammars in which rule references include direct or indirect self-reference.

...but it is definitely nice to have (wait til you see my next PR :wink:)

@bklang Thanks, that's great. I'll work on style-fixes of the code I changed.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.09%) when pulling b5ebfbfa57672f253be9e670f342386883859714 on sfgeorge:feature/ruleref-nesting into 303f90fcbb87dd235fb3a49ffa0c197cf8451eac on benlangfeld:develop.

bklang commented 8 years ago

There's one failing spec on JRuby. Is that a false positive due to XML libraries, or is it real?

sfgeorge commented 8 years ago

Yes, it appears to be related to XML libraries... specifically nokogiri. ruby_speech's most recent build to pass on JRuby was https://travis-ci.org/benlangfeld/ruby_speech/jobs/23457168. It wasn't the code or spec differences that were significant, but rather version of nokogiri used (1.6.1 at the time)

From my tests, 4 pre-existing ruby_speech specs do not build successfully with nokogiri >= 1.6.2. I guess we either should lock-down nokogiri in the gemspec, or update the 4 specs to work with later versions of nokogiri. If we do the latter, we may want to warn users of ruby_speech on how to avoid the same xml attribute problems that our tests hit.

Versions of nokogiri in ruby_speech.gemspec that I tested with jruby-1.7.9:

#  s.add_runtime_dependency %q<nokogiri>, ["= 1.5.10"]    # PASSES
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.5.11"]    # PASSES
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.0"]     # PASSES
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.1"]     # PASSES
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.2"]     # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.2.1"]   # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.3"]     # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.3.1"]   # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.4"]     # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.4.1"]   # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.5"]     # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.6.1"]   # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.6.2"]   # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.7.rc2"] # FAILS
#  s.add_runtime_dependency %q<nokogiri>, ["= 1.6.7.rc3"] # FAILS
sfgeorge commented 7 years ago

Superseded by #39. Closing.