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

Added support for creating a Jsgf object from a string #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bfoz
Copy link

@bfoz bfoz commented Feb 6, 2015

This allows for the use of JSGF grammars that don't live in files, or are otherwise generated elsewhere.

This implementation abuses optional arguments and therefore may not be the best way to do it, but I wasn't sure what exactly you'd prefer. Jsgf::initialize already takes a string parameter that it expects to be the filename, which makes it tough to simply pass a grammar string. So I added an optional second parameter ('jsgf') and a class-level convenience-method so that nobody needs to know about the new parameter. I'm happy to change the implementation if you prefer something else.

For those times when you already have a JSGF grammar in a string
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.52% when pulling e6a04d0 on bfoz:jsgf_string into 55664f7 on watsonbox:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.52% when pulling e6a04d0 on bfoz:jsgf_string into 55664f7 on watsonbox:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.52% when pulling e6a04d0 on bfoz:jsgf_string into 55664f7 on watsonbox:master.

@watsonbox
Copy link
Owner

I'm fine with this but I think I'd prefer a more OO approach for these two different behaviors. Perhaps a JsgfString and a JsgfFile which are both Jsgf but with different initializers?

@bfoz
Copy link
Author

bfoz commented Feb 8, 2015

Interesting. I was trying to avoid that level of breakage because I thought you wouldn't accept it.

How would JsgfFile differ from the current Jsgf, considering that it currently just loads a file? Are you envisioning that the Jsgf initializer would be changed to take no arguments, and then the subclass initializers set the parent's raw attribute? Or would Jsgf initialize with a string?

FWIW, one thing that I had considered was changing Jsgf to take a string initializer for it's content, and then using a class method for loading a file...

jsgf = Jsgf.read('filename')    # Also accepts an IO object
jsgf = Jsgf.new('...contents...')

That seemed a little more IO-like to me, and wouldn't require the user to think about which subclass to use. But it would also cause a lot of breakage, or at least force a bump of the major version number (if you're using semantic versioning).

Using named arguments (or 1.9.x's pseudo named arguments) is probably the more ruby-typical approach to this, but again with the breakage.

@watsonbox
Copy link
Owner

Yes Jsgf could initialize with a raw grammar string and then JsgfFile might set that from a file. Then any other implementations (JsgfService?!) would follow a similar pattern.

To be honest, since any development CMU Sphinx API change can currently force a change to pocketsphinx-ruby, I'm not too concerned about breaking changes. Of course, I'd try to avoid them, but the changes in question aren't even really part of the main documented API. I expect that once stable Sphinx releases are available I'll release a 1.0 version and be more strict about things from then on.

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.

3 participants