Added DifferenceOfSquares Test Generator #442

felix91gr · 2017-09-28T18:54:06Z

Thank you @jpreese for the help :)

I think I'm starting understand the basics of how the Exercises framework works :D

ErikSchierboom · 2017-09-28T19:32:28Z

@felix91gr Just to be safe: did you run the test generator project? That is, did you run dotnet run in the generators directory? You might have forgotten that, as I get a compile error when I run your branch on my machine. This compile error is due to a problem in the test generators that was just fixed on the master branch.

What you should do is to rebase your branch on the latest master and then try to run the generator again. If there is no compile error, the test generator should run. Usually, the test file generated by the newly added generator is slightly different from what it was, which I can confirm is the case once you have done the rebase and re-run the generator.

When the generated tests file is different, you have to check to see if the example and default (stub) implementation of that exercise are still correct. You can verify that my running the build script (build.ps1 or build.sh, depending on your operating system).

If you have any further questions, feel free to ask!

felix91gr · 2017-09-28T19:46:35Z

Just to be safe: did you run the test generator project? That is, did you run dotnet run in the generators directory?

No, I didn't, I didn't know. I should've asked. What I did do was run dotnet test in the generators/Exercises/ directory.

This compile error is due to a problem in the test generators that was just fixed on the master branch.

Ohhh, I see. Thanks! I'll rebase it first then :)

If you have any further questions, feel free to ask!

I will! Thank you so much :)

felix91gr · 2017-09-28T20:10:37Z

I did the rebase.

But I don't know if the PR now works... this Starbucks' internet flaked out and I think NuGet needs a stable connection to work. NuGet gets invoked when I write run dotnet for some reason.
If Travis passes, it's good?

Anyhow... when I'm back home I'll fix it if it doesn't work :)

ErikSchierboom · 2017-09-28T20:50:48Z

dotnet run indeed needs an internet connection as it is downloading the Nuget packages. Having the Travis build pass is not enough unfortunately, as it currently does not run the generators project.

Good luck fixing things and we really appreciate the help!

felix91gr · 2017-09-29T01:54:42Z

Now I ran dotnet run without any failures. It generated everything allright.

What should I check next before the merge?

Edit

I was re-reading your first response, and I think you already answered this question :P

You can verify that my running the build script

I'm doing so as we speak! :)

Edit 2

build.sh failed on me, but I'm really drained from today's work... so I'll continue tomorrow :)

ErikSchierboom

@felix91gr Aha, progress! I can now see the updated DifferenceOfSquaresTest.cs file, so your generator is working. There are a couple of issues that cause your build to fail:

By running dotnet run, all generators run and some other exercises have had their test data updated leading to update test files. The updated data could very well break the existing example. This is actually my fault, as I should have mentioned the option to run the generator for a single exercise by using the -e parameter. Hence, instead of running dotnet run, you should run dotnet run -e difference-of-squares. This will ensure that only the difference of squares test generator is being run.
For this PR, you should remove the files not directly related to your new test generator. That means the following files should be removed: exercises/largest-series-product/LargestSeriesProductTest.cs, exercises/pig-latin/PigLatinTest.cs, exercises/sum-of-multiples/SumOfMultiplesTest.cs, generators/Exercises/Clock.cs (which should not be there due to the rebase).
As you can see in the diff, the updated DifferenceOfSquaresTest.cs file expects a different API than the current implementation. Whereas previously the tested methods were part of a class named Squares, the updated API expects it to be called DifferenceOfSquares. You should rename the example and stub implementation files to match the updated class name,

With the above changes, I think your PR will be done!

felix91gr · 2017-09-29T20:21:20Z

I changed and tested the test generator and the tests generated. Git didn't let me work without automatically changing the files that have CRLF, replacing it with LF, so... there's that 🤔

But everything else worked just fine :)

Btw: I changed the name of the class in the test to Squares, because it better fitted the rest of the code in the exercise ^^

ErikSchierboom

Ah, I see that you changed the name of the test generator class, but that won't work as the test generator name has to match the exercise name. So could you revert this back to DifferenceOfSquares? You should then modify the example and stub implementation's class names, as explained in my previous comment. Good luck!

ErikSchierboom

Also, could you remove all files not related to the DifferenceOfSquares exercise from the PR?

felix91gr · 2017-09-29T20:52:37Z

So could you revert this back to DifferenceOfSquares?

Yeah, sure! :)

could you remove all files not related to the DifferenceOfSquares exercise from the PR?

I'm not sure. git isn't letting me go on without changing those files. I think that when they are cloned, they aren't written as-is to my machine. I try to git stash them, but nothing happens.

The problem lies within the CRLF combination (aka \r\n). I'll investigate a bit more on this. As I am now, I don't know how to commit without touching them on Linux.

felix91gr · 2017-09-29T20:57:38Z

🤔 do we have a git attributes file? It seems we could streamline the CRLF v/s LF process with one of those :)

ErikSchierboom · 2017-09-29T21:09:04Z

I'm off to bed now. I'll have some tips how to deal with the git issues tomorrow!

felix91gr · 2017-09-30T01:26:20Z

Speaking of this:

as the test generator name has to match the exercise name

🤔 we might have to change the canonical-data.json file for this one.

Let me explain: using the name DifferenceOfSquares for the class isn't allowed, as the class has a member with the same name:

public static int DifferenceOfSquares(int max)
{
    throw new NotImplementedException("You need to implement this function.");
}

With the class being named the same as this method, I get the following error while running build.sh:

DifferenceOfSquares.cs(19,23): error CS0542: 'DifferenceOfSquares': member names cannot be the same as their enclosing type [/home/felix/Documents/Programming/exercism/csharp/build/difference-of-squares/DifferenceOfSquares.csproj]

And if I have understood the framework correctly, the method name can't change unless the property name described in the canonical-data.json file changes as well.

What do you think? Should we keep the Tester Generator being named Squares, or should we modify the canonical data file?

ErikSchierboom · 2017-09-30T07:20:54Z

What do you think? Should we keep the Tester Generator being named Squares, or should we modify the canonical data file?

Excellent question. In general, we never modify the canonical data itself (which we don't control), we just modify our internal representation (which we do control). An example of this is the test generator for the leap exercise, which changes the tested method to a different name by overriding the UpdateCanonicalData method:

public class Leap : Exercise
{
    protected override void UpdateCanonicalData(CanonicalData canonicalData)
    {
        foreach (var canonicalDataCase in canonicalData.Cases)
            canonicalDataCase.Property = "IsLeapYear";
    }
}

I think in our case we have two options:

Use a different name for the class (e.g. Squares). However, I would then also would like the test file named SquaresTests, which would require a modification to the generator framework.
Use a different for the tested method. I'm in favor of this option, where we rename the tested method from DifferenceOfSquares to CalculateDifferenceOfSquares (and I think we should then also change SumOfSquares to CalculateSumOfSquares).

If you agree with me and go for option 2, you should be able to do this using the same approach used in the Leap exercise (as shown above)

do we have a git attributes file? It seems we could streamline the CRLF v/s LF process with one of those :)

We do have a .gitattributes file, and I thought that this worked well on my Mac. I'll have to check another time.

I'm not sure. git isn't letting me go on without changing those files. I think that when they are cloned, they aren't written as-is to my machine. I try to git stash them, but nothing happens.

You shouldn't change the files themselves, you should just use git to revert them to their original version. You can do this using git checkout, but you should first make sure you are up to date with the latest version of this repository. So we'll start with doing that:

git remote add exercism https://github.com/exercism/csharp
git fetch exercism
git rebase exercism/master

You can skip the first line if you already have add the exercism remote.

We then reset the files we don't want to the version in the exercism remote:

git checkout exercism/master exercises/largest-series-product/Example.cs
git checkout exercism/master exercises/largest-series-product/LargestSeriesProductTest.cs
git checkout exercism/master generators/Exercises/Clock.cs
git add exercises/largest-series-product/Example.cs
git add exercises/largest-series-product/LargestSeriesProductTest.cs
git add generators/Exercises/Clock.cs
git commit --amend
git push --force

This should remove the changes in those files from your PR. As long as you never run dotnet run, but always run dotnet run -e difference-of-squares, those changes will not come back.

felix91gr · 2017-09-30T19:11:15Z

(...) If you agree with me and go for option 2 (...)

Awesome explanation! I get it better now, and I completely agree. I'll do that.

We do have a .gitattributes file,

Hmm. It looks like the eol for * files isn't predefined. That might be the problem. One of the commits in the repo's history put CRLF endings in these two files, or something like that:

~/D/P/exercism $ git clone https://github.com/exercism/csharp/ csharp_original
# blablabla
~/D/P/exercism $ cd csharp_original/
~/D/P/e/csharp_original (master) $ git status
On branch master
Your branch is up-to-date with 'origin/master'.
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   exercises/largest-series-product/Example.cs
	modified:   exercises/largest-series-product/LargestSeriesProductTest.cs

no changes added to commit (use "git add" and/or "git commit -a")
~/D/P/e/csharp_original (master) $ git add .
warning: CRLF will be replaced by LF in exercises/largest-series-product/Example.cs.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in exercises/largest-series-product/LargestSeriesProductTest.cs.
The file will have its original line endings in your working directory.

Git's messages are a bit ambiguous here. But I think I'm understanding them allright 🤔

The CRLF endings must be messing with my computer, which has Git set it to default to LF. I think we might be better off picking a default line ending, and saying goodbye to this problem once and for all 🤔

What do you think?

ErikSchierboom · 2017-10-01T07:04:22Z

I think we might be better off picking a default line ending, and saying goodbye to this problem once and for all

@robkeim @jpreese What do you think? Should we just use \n as the line endings?

jpreese · 2017-10-02T01:38:46Z

As long as we choose one, for consistency, I don't really think it matters. https://help.github.com/articles/dealing-with-line-endings/

ErikSchierboom · 2017-10-02T06:07:22Z

@felix91gr Okay, we'll solve the git issue in another issue. Could you cleanup this PR as requested (to remove the extra files)?

felix91gr · 2017-10-02T06:17:09Z

Awesome! And yes, I will :) I’ll be back in a couple of days: atm, I’m kinda busy and also kinda sleep deprived xD I hope I’ll be back by... Wednesday, or so. See ya!

…

On Mon, Oct 2, 2017 at 3:07 AM Erik Schierboom ***@***.***> wrote: @felix91gr <https://github.com/felix91gr> Okay, we'll solve the git issue in another issue. Could you cleanup this PR as requested (to remove the extra files)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#442 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALNBJ0HH3xmDfD-VxOhLvKZTYNZxpNXDks5soH2bgaJpZM4Pnug-> .

robkeim · 2017-10-02T16:16:46Z

@ErikSchierboom - I'm with @jpreese and as long as we have consistent way of doing things that sounds good to me! I use Windows and every editor that I know renders \n correctly these days so I don't see any potential concerns from people trying to contribute on Windows if we go that direction.

ErikSchierboom · 2017-10-03T07:07:05Z

I've created an issue for the line-ending issue: #452

jpreese · 2017-10-10T01:12:49Z

Did you need any help with this @felix91gr ?

felix91gr · 2017-10-10T01:18:33Z

🤔 I shouldn’t be having more git issues, and therefore this PR should be streamlined from now on. I’ll try to finish it during this week... I’ll ask you for help if I’m stuck ^^

…

On Mon, Oct 9, 2017 at 10:12 PM John Reese ***@***.***> wrote: Did you need any help with this @felix91gr <https://github.com/felix91gr> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#442 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALNBJ0bSw0G6x-BlZOTbsi9956LUF1mjks5sqsSSgaJpZM4Pnug-> .

felix91gr · 2017-10-10T23:28:10Z

I deleted my fork and started over again. I will link to this PR as reference from a new PR

jpreese · 2017-10-10T23:32:29Z

You can push your changes to the same branch -- no need to close and open a new PR.

felix91gr · 2017-10-10T23:34:44Z

Yeah... but I messed up too many things:

Used master instead of a new branch
Merged progress made in exercism/csharp/master and put it as my commit
Made spurious commits with my deprecated git

I get what you mean. But I really would prefer to start this one again clean.

- 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

- 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 #442) - Changed names of methods required in exercise to fit new names generated in tests

Added DifferenceOfSquares Test Generator

6e232ad

Fix clock generator (#440) (#1)

7e0e094

ErikSchierboom requested changes Sep 29, 2017

View reviewed changes

Changed the test generator to fit the pre-existing class names

d87ccb7

ErikSchierboom requested changes Sep 29, 2017

View reviewed changes

felix91gr closed this Oct 10, 2017

felix91gr mentioned this pull request Oct 10, 2017

Added DifferenceOfSquares generator #471

Merged

felix91gr mentioned this pull request Oct 11, 2017

Created Generator for DifferenceOfSquares (#471) felix91gr/csharp#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added DifferenceOfSquares Test Generator #442

Added DifferenceOfSquares Test Generator #442

felix91gr commented Sep 28, 2017

ErikSchierboom commented Sep 28, 2017

felix91gr commented Sep 28, 2017

felix91gr commented Sep 28, 2017 •

edited

Loading

ErikSchierboom commented Sep 28, 2017

felix91gr commented Sep 29, 2017 •

edited

Loading

ErikSchierboom left a comment

felix91gr commented Sep 29, 2017 •

edited

Loading

ErikSchierboom left a comment

ErikSchierboom left a comment

felix91gr commented Sep 29, 2017 •

edited

Loading

felix91gr commented Sep 29, 2017

ErikSchierboom commented Sep 29, 2017

felix91gr commented Sep 30, 2017

ErikSchierboom commented Sep 30, 2017 •

edited

Loading

felix91gr commented Sep 30, 2017

ErikSchierboom commented Oct 1, 2017

jpreese commented Oct 2, 2017

ErikSchierboom commented Oct 2, 2017

felix91gr commented Oct 2, 2017 via email

robkeim commented Oct 2, 2017

ErikSchierboom commented Oct 3, 2017

jpreese commented Oct 10, 2017

felix91gr commented Oct 10, 2017 via email

felix91gr commented Oct 10, 2017

jpreese commented Oct 10, 2017

felix91gr commented Oct 10, 2017

Added DifferenceOfSquares Test Generator #442

Added DifferenceOfSquares Test Generator #442

Conversation

felix91gr commented Sep 28, 2017

ErikSchierboom commented Sep 28, 2017

felix91gr commented Sep 28, 2017

felix91gr commented Sep 28, 2017 • edited Loading

ErikSchierboom commented Sep 28, 2017

felix91gr commented Sep 29, 2017 • edited Loading

Edit

Edit 2

ErikSchierboom left a comment

Choose a reason for hiding this comment

felix91gr commented Sep 29, 2017 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

felix91gr commented Sep 29, 2017 • edited Loading

felix91gr commented Sep 29, 2017

ErikSchierboom commented Sep 29, 2017

felix91gr commented Sep 30, 2017

ErikSchierboom commented Sep 30, 2017 • edited Loading

felix91gr commented Sep 30, 2017

ErikSchierboom commented Oct 1, 2017

jpreese commented Oct 2, 2017

ErikSchierboom commented Oct 2, 2017

felix91gr commented Oct 2, 2017 via email

robkeim commented Oct 2, 2017

ErikSchierboom commented Oct 3, 2017

jpreese commented Oct 10, 2017

felix91gr commented Oct 10, 2017 via email

felix91gr commented Oct 10, 2017

jpreese commented Oct 10, 2017

felix91gr commented Oct 10, 2017

felix91gr commented Sep 28, 2017 •

edited

Loading

felix91gr commented Sep 29, 2017 •

edited

Loading

felix91gr commented Sep 29, 2017 •

edited

Loading

felix91gr commented Sep 29, 2017 •

edited

Loading

ErikSchierboom commented Sep 30, 2017 •

edited

Loading