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

Vocabulary interface missing an iterable or equivalent method #904

Closed
grosenberg opened this issue Jun 15, 2015 · 8 comments
Closed

Vocabulary interface missing an iterable or equivalent method #904

grosenberg opened this issue Jun 15, 2015 · 8 comments

Comments

@grosenberg
Copy link

Depreciation of Recognizer.getTokenNames() presently leaves no equivalent or convenient method to iterate over the valid set of symbolic token names. Since the underlying String[] is now private, cannot iterate based on size.

Adding a Vocabulary.getSymbolicNames() method, returning a String[], would be helpful.

@sharwell
Copy link
Member

💡 ATN.maxTokenType should provide you with the upper bound.

@parrt
Copy link
Member

parrt commented Jun 20, 2015

I agree that we should have a way to iterate or get the list but I don't think we can change the interface unless we make a major version, such as 4.6 but I'm heading to 4.5.1 shortly. @sharwell do you have a suggestion for an enhancement to the interface?

@sharwell
Copy link
Member

We can certainly update the VocabularyImpl class, and you can change the generated VOCABULARY field to be an instance of VocabularyImpl instead of just Vocabulary.

As for the specific API, I'm not sure yet because Vocabulary is meant to expose multiple different pieces of information at each index, but iterating over it would only provide a single piece of information.

@grosenberg
Copy link
Author

@sharwell @parrt - the need for a list or iterable occurs, e.g., in an editor to enable quick fix help as symbolic names are being typed in. Thus the need/use is not index related. Just the single method returning the symbolic names is needed.

@parrt
Copy link
Member

parrt commented Oct 12, 2015

ha! I just needed this myself.

On Sat, Jun 20, 2015 at 2:54 PM, GRosenberg notifications@github.com
wrote:

@sharwell https://github.com/sharwell @parrt https://github.com/parrt

  • the need for a list or iterable occurs, e.g., in an editor to enable
    quick fix help as symbolic names are being typed in. Thus the need/use is
    not index related. Just the single method returning the symbolic names is
    needed.


Reply to this email directly or view it on GitHub
#904 (comment).

Dictation in use. Please excuse homophones, malapropisms, and nonsense.

@msteiger
Copy link
Contributor

Same problem here. I have two three alternative suggestions that would at least circumvent the issue that the user might be interested in any of the three names:

int Vocabulary.getTokenCount() - this would allow for index-based iterations and is a simple alternative to the deprecated method. This would restrict the Vocabulary token type values to the range [0..n] though.

int[] Vocabulary.getTokenIndices() - this would provide an arbitrary collection of ints, which would map to the range [0..n] for all existing cases, but supports other token type values, such as -1 as well.

List<Integer> Vocabulary.getTokenIndices() - similar, but more nice features such as immutability, toString and stream support. It could be backed by an AbstractList that computes (immutable) integer entries on the fly.

I would probably go for the last proposed suggestion as it the most flexible and elegant one. I can draft the required code changes and submit a pull request, if desired.

@parrt
Copy link
Member

parrt commented Mar 24, 2016

Seems like getTokenCount() would be enough; only EOF is -1 and we could specify that as a special case. Or maybe getMaxTokenType() as there is no requirement that all token types are used. getSymbolicNames() would also be dang convenient. Can methods be added to an interface and be binary compatible with code that currently refs the interface?

@msteiger
Copy link
Contributor

Yes, they are compatible (I checked with a quick test). While an Iterable for symbolic names would be convenient at first, it might not be ideal as you cannot get the corresponding literal name from it. If iterating over names is a common use case, it might make sense though, imho.

Alternative approach: a triple that contains symbolic, literal and display name and an Iterable that can be acquired from Vocabulary. This would give all three corresponding names for each entry.

@parrt parrt added this to the 4.5.3 milestone Mar 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants