Skip to content

refinements: removed from individual cases#697

Merged
Insti merged 1 commit intoexercism:masterfrom
ajwann:remove-refinements-from-individual-cases
Aug 8, 2017
Merged

refinements: removed from individual cases#697
Insti merged 1 commit intoexercism:masterfrom
ajwann:remove-refinements-from-individual-cases

Conversation

@ajwann
Copy link
Copy Markdown
Contributor

@ajwann ajwann commented Jul 28, 2017

fixes #645

Comment thread lib/generator/exercise_case.rb Outdated
protected

def underscore(target)
target.underscore
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

ajwann commented Jul 28, 2017 via email

@Insti
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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