-
Notifications
You must be signed in to change notification settings - Fork 29
[Event Highlighting]: White label minus, dot and colon chars in mini notation #224
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
[Event Highlighting]: White label minus, dot and colon chars in mini notation #224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good overall, I'd just suggested two cosmetic comments
lib/line-processor.js
Outdated
const digitMin = 48; | ||
const digitMax = 57; | ||
// A-Z | ||
const upperCaseMin = 65; | ||
const upperCaseMax = 90; | ||
// a-z | ||
const lowerCaseMin = 97; | ||
const lowerCaseMax = 122; | ||
// . | ||
const dot = 46; | ||
// - | ||
const minus = 45; | ||
// : | ||
const colon = 58; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these const could be kept at the class level, so it will keep the isValidTidalWordChar
tidier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! what a pity that in js there's the need to prepone the class name on every constant call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
lib/line-processor.js
Outdated
static isQuotationMark(character) { | ||
const code = character.charCodeAt(0); | ||
// " | ||
const quotationMark = 34; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This will fix the following issues:
General speaking we need to whitelabel chars that are considered as part of a word inside of the mini notation. I added an additional check, that will avoid that the event highlighting crashes because of a wrong tokenization.