-
Notifications
You must be signed in to change notification settings - Fork 29
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
KeySignature - port to TypeScript #22
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.
@meganindya Currently, this PR ports only a part of the file, so that its easier to go through line by line. Please have a look at the ported and original code and share your insights. I have also highlighted some points I was curious about. Thanks
This is taking a while. I'm making a few changes on top. Converting to draft for now. |
Improve on the documentation (plus cosmetic changes) extensively. Add minor code improvements and corrections sparingly.
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.
@walterbender need some inputs.
Documentation required at some more places. I've marked the areas with a 'docs' comment.
On another note, there are 16 print
statements in the Python code. I was thinking if we should use throw
-try
-catch
error handling for those cases instead?
'ajam maqam': 'bb' | ||
}; | ||
|
||
static readonly MODE_MAPPER: { [key: string]: string } = { |
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.
docs
key: string; | ||
mode: string; | ||
scale: string[]; | ||
halfSteps: number[]; | ||
keySignature: string; | ||
genericScale: string[]; | ||
scalarModeNumbers: string[]; | ||
eastIndianSolfegeNotes: string[]; |
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.
Which of these should we expect to be directly accessed through the instance, and which are private?
Also need docs.
private _scale: Scale; | ||
private _noteNames: string[]; | ||
private _fixedSolfege: boolean; | ||
private _solfegeNotes: string[]; | ||
private _customNoteNames: string[]; | ||
private _numberOfSemitones: number; |
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.
docs
Add getters for public instance variables, add missing documentation, and make cosmetic improvements.
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.
Took me forever to finish this up. Great job on your part though.
Reference #18
Ports
keysignature.py
to typescript.