Skip to content

refinements: removed from individual cases #697

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

Merged

Conversation

ajwann
Copy link
Contributor

@ajwann ajwann commented Jul 28, 2017

fixes #645

protected

def underscore(target)
target.underscore
Copy link
Contributor Author

@ajwann ajwann Jul 28, 2017

Choose a reason for hiding this comment

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

I think it's best if we still use the refinement within ExerciseCase. If we don't, we will have to define two separate underscore methods, one for Fixnum and one for String, or define a single underscore method that switches on the type of the argument. Personally I strongly dislike the latter option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that switching on type is undesirable.

@Insti
Copy link
Contributor

Insti commented Jul 28, 2017

Getting rid of the refinements in the Case classes is good 👍 and I think what you have here is mergable.

I'm not sure continuing to use the function with refinements the way we are here is a good idea. Underscoring a string and underscoring an Integer are two fundamentally different operations performed for different reasons, yet they are called using the same function.

Perhaps two different methods is the right way to go, but naming them based on why rather than what.

@kotp had some thoughts about this (that I can't find to link to right now.)

Edit: What about literal(number) and underscore(string)?

@ajwann
Copy link
Contributor Author

ajwann commented Jul 28, 2017 via email

@Insti
Copy link
Contributor

Insti commented Jul 29, 2017

I'm in favor of creating the literal(number) and underscore(string) methods.

Is this something you want to do as part of this PR?

@ajwann
Copy link
Contributor Author

ajwann commented Jul 29, 2017

@Insti sure, no problem! I can make the change right now actually.

@ajwann ajwann force-pushed the remove-refinements-from-individual-cases branch from da7277a to ed73275 Compare July 29, 2017 23:36
@ajwann
Copy link
Contributor Author

ajwann commented Aug 6, 2017

@Insti I think this is good to, but let me know if you want any more changes.

@ajwann ajwann force-pushed the remove-refinements-from-individual-cases branch from ed73275 to 74da043 Compare August 7, 2017 22:14
@Insti Insti merged commit 55a88f3 into exercism:master Aug 8, 2017
@Insti
Copy link
Contributor

Insti commented Aug 8, 2017

Looks good, thanks @ajwann

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.

generator: refinements and lexical scope.
2 participants