Add support for alphanumeric labels #39
Conversation
louga31
changed the title
aebabis
left a comment
aebabis
left a comment
There was a problem hiding this comment.
Thanks for expressing interest in improving this project. I agree this is a good feature to add, but the new forEach loops are verbose, and I think we should try to eliminate them before merging, if possible
|
Sorry. I didn't notice your PR comment, only your commit messages; now I understand you're aware there are bugs. I don't know if I'll have the time to locate and fix the bug, but if you can add a 3rd test that should pass but fails, I'll at least take a stab at it. |
I will add tests for this and the mix of named and integer labels |
|
The lastest commit fixes the issue, now I need to improve the code and do some cleanup and it should be ready |
| .filter(({state}) => typeof state === 'number') | ||
| .map(({statements}) => statements.map(s => s.start + s.operations.length)) | ||
| .flat(); | ||
| return getMaxRounded(ends); |
There was a problem hiding this comment.
This all looks really good. One final suggestion:
| return getMaxRounded(ends); | |
| return Math.max(0, ...ends) + 1; |
I don't think it's necessary to threshold the label to 10 since the user never sees the generated state numbers. Taking it out would remove the need for the helper function.
Unless.... we wanted to make every generated state be thresholded so that IDEs could give the information in tooltips using static code analysis. Would this be a useful feature? Because right now, we're just thresholding the first string-labelled state which doesn't make a lot of sense.
There was a problem hiding this comment.
The code was rounding all the generated labels to the next multiple of 10 (10, 20, ...) but yeah, this is not strictly necessary, so I removed it
|
@louga31 I pulled this down and tried to fix it. I struggled because I've forgotten how to deal with lexer ambiguities and don't currently have time to relearn. Would you happen to know anyone who can look at |
2 participants
This pull request add support for alphanumeric labels.
I have added tests to it.
Currently, it only works with labels without numbers, I'm new to the Moo library and I couldn't get it to match my string "type", it always match number or id (if I put string above them it breaks everything).
If this parsing issue is fixed, the rest of the code should support label names with numbers.