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

Fix +at(slice-name) #29

Merged
merged 3 commits into from
Aug 21, 2014
Merged

Fix +at(slice-name) #29

merged 3 commits into from
Aug 21, 2014

Conversation

Djules
Copy link
Contributor

@Djules Djules commented Aug 20, 2014

Fix the related bug when using +at() mixin with the slice's name instead
of its measure

Fix the related bug when using +at() mixin with the slice's name instead
of its measure
@jescalan
Copy link
Owner

This looks good to me @Djules! Thanks for also going the extra mile and adding tests. As long as @declandewet signs off on this I would say we're good to merge. Thanks for the excellent contribution!

@Djules
Copy link
Contributor Author

Djules commented Aug 20, 2014

I actually found an error when using +at('xl') so please wait for the next merge...

Fix previously inserted bug wen using mixin +at('xl')
@jescalan
Copy link
Owner

Cool, is this ready now @Djules?

@Djules
Copy link
Contributor Author

Djules commented Aug 21, 2014

Exactly @Jenius !

The private function -get-index() should return the number of the slice (start at 1) instead of its zero-based index. Maybe you should rename it as -get-scale-number or something like that.

@zspecza
Copy link
Collaborator

zspecza commented Aug 21, 2014

This looks good - I like the idea of changing the name of that function if you think it might read better. I dont have the time to do it myself so if you can include that change here then this will be ready to merge, otherwise it's not such a big deal and can be merged in anyway. Up to you @Djules

@jescalan
Copy link
Owner

Awesome, let me know if/when that function name is changed and we can get this merged up 👍

The function -get-index() has been renamed to -get-scale-number(), as it
actually returns the scale number from its name, and not a zero-based
index.
@Djules
Copy link
Contributor Author

Djules commented Aug 21, 2014

So I renamed the function to -get-scale-number and I have removed its second argument, because I don't see the point of a loosely coupled function if this function returns no more than a position of a scale in a list, but I may be wrong with that.

Let me know what you think. :)

@jescalan
Copy link
Owner

Yup, that seems like the right approach to me. Don't implement until you need it.

jescalan pushed a commit that referenced this pull request Aug 21, 2014
@jescalan jescalan merged commit 3214bc6 into jescalan:master Aug 21, 2014
@jescalan jescalan mentioned this pull request Aug 21, 2014
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.

3 participants