Skip to content

Added DifferenceOfSquares generator #471

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

felix91gr
Copy link
Contributor

Work I have left to do is written on #442. I messed up that branch too much, so I deleted my fork and started it again.

@felix91gr felix91gr force-pushed the adding-diff-of-squares-generator branch from 5199f53 to 757f286 Compare October 11, 2017 00:25
@felix91gr felix91gr changed the title [WIP] Adding DifferenceOfSquares generator Added DifferenceOfSquares generator Oct 11, 2017
@felix91gr
Copy link
Contributor Author

felix91gr commented Oct 11, 2017

I finished testing this on my machine.

@ErikSchierboom and @jpreese please review it :)

@felix91gr
Copy link
Contributor Author

felix91gr commented Oct 11, 2017

BTW, is there a way to run only one exercise's test suite? To wait less when I'm debugging my changes to one of them? (right now, I'm working on the largest-series-product one) I figured this one out xD

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I have two small change requests, but other than that it looks fantastic!

canonicalDataCase.Property = "CalculateDifferenceOfSquares";
break;
default:
canonicalDataCase.Property = "Default_This_Shouldnt_Happen";
Copy link
Member

Choose a reason for hiding this comment

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

Could you throw an exception here?

Copy link
Member

Choose a reason for hiding this comment

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

The formatting of this switch is off (see the diff in GitHub).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Um, that default shouldn't even exist, actually. I just put it there to debug my changes to the property names. If another property comes, and has a new name, it shouldn't make the generator throw. Since this loop is an override of the original names, it should just ignore new names that come up.

@felix91gr felix91gr force-pushed the adding-diff-of-squares-generator branch from 757f286 to 5f39509 Compare October 11, 2017 19:03
@felix91gr
Copy link
Contributor Author

There we go. I removed the default. And about this...

The formatting of this switch is off (see the diff in GitHub).

I think I know what you mean. I've just found the diff (I was gonna ask you about it). Lemme fix it.

@felix91gr felix91gr force-pushed the adding-diff-of-squares-generator branch from 5f39509 to 0e26a45 Compare October 11, 2017 19:07
- Changed name of class for exercise from Squares to DifferenceOfSquares, as per the standards
- Changed names of methods generated in tests to fit restrictions of naming in C# (see PR exercism#442)
- Changed names of methods required in exercise to fit new names generated in tests
@felix91gr felix91gr force-pushed the adding-diff-of-squares-generator branch from 0e26a45 to 99c52d1 Compare October 11, 2017 19:09
@felix91gr
Copy link
Contributor Author

There we go. It was tabs v/s spaces once again.

@ErikSchierboom: it's ready for revision :)

@ErikSchierboom ErikSchierboom merged commit c7a19ff into exercism:master Oct 11, 2017
@ErikSchierboom
Copy link
Member

Great, thanks a lot! 🎉

felix91gr added a commit to felix91gr/csharp that referenced this pull request Oct 11, 2017
Created Generator for DifferenceOfSquares (exercism#471)
@felix91gr felix91gr deleted the adding-diff-of-squares-generator branch October 11, 2017 19:56
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