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

Implement pitch.getEnharmonic(). #249

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gregchapman-dev
Copy link

No description provided.

Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Greg! Congrats on the first PR. Can you add a test to js_tests/module_tests/pitch (just follow the formula there) to make sure that it works as expected w/ both inPlace=True and not inPlace=True.

grunt test is your friend to make sure that tests pass.

Thanks! Other comments below.

src/pitch.ts Show resolved Hide resolved
src/pitch.ts Outdated Show resolved Hide resolved
src/pitch.ts Outdated Show resolved Hide resolved
src/pitch.ts Outdated Show resolved Hide resolved
Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Greg. I think you misunderstood. We should return the new pitch if not inPlace or the original if inPlace. There is no point in returning "this" if all the manipulations were done to "p" instead

@gregchapman-dev
Copy link
Author

In _getEnharmonicHelper, p is self.clone(). Everything is done to p, which is then returned if not inPlace. If inPlace, everything is copied from p to self, which is then returned. I believe this works correctly. I actually prefer the pattern I used in getEnharmonic (post is either self or self.clone(), and you do everything to post, and return post); would you like that pattern in _getEnharmonicHelper?

@gregchapman-dev
Copy link
Author

@mscuthbert Do you agree that _getEnharmonicHelper is actually correct? Or would you rather I follow getEnharmonic's pattern?

@mscuthbert
Copy link
Member

Sorry not to reply -- I prefer your way of doing it.

@gregchapman-dev
Copy link
Author

I can't tell for sure, do you want a change or not?

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.

2 participants