Add analyzer for weighing-machine #109

Grenkin1988 · 2021-11-13T22:09:44Z

Fixes: #107

Grenkin1988 · 2021-11-13T22:11:06Z

This is the start, but would be nice to hear feedback if I am doing this right.
Do I need to add some additional analysis?

ErikSchierboom

This is great! I've left some small comments.

src/Exercism.Analyzers.CSharp/Analyzers/WeighingMachine/WeighingMachineAnalyzer.cs

src/Exercism.Analyzers.CSharp/Analyzers/Shared/SharedComments.cs

src/Exercism.Analyzers.CSharp/Analyzers/WeighingMachine/WeighingMachineAnalyzer.cs

src/Exercism.Analyzers.CSharp/Analyzers/WeighingMachine/WeighingMachineSyntaxFactory.cs

src/Exercism.Analyzers.CSharp/Syntax/PropertyDeclarationSyntaxExtensions.cs

src/Exercism.Analyzers.CSharp/Analyzers/WeighingMachine/WeighingMachineSolution.cs

Grenkin1988 · 2021-11-16T23:01:28Z

Oh, now we need to use VS2022

src/Exercism.Analyzers.CSharp/Properties/launchSettings.json

ErikSchierboom · 2021-11-17T07:54:15Z

Oh, now we need to use VS2022

Sure. I'm using Rider BTW.

Grenkin1988 · 2021-11-17T19:19:42Z

Comments: exercism/website-copy#2134

ErikSchierboom · 2021-11-18T08:26:34Z

@Grenkin1988 Great work! Before I'll start reviewing, there's one thing missing: tests. With an analyzer, it is really important to have a big set of tests, as we don't want to give the wrong suggestions. See https://github.com/exercism/csharp-analyzer/tree/main/test/Exercism.Analyzers.CSharp.IntegrationTests/Solutions/Gigasecond for an example of what a test suite for an analyzer looks like.

Grenkin1988 · 2021-11-18T08:47:30Z

@Grenkin1988 Great work! Before I'll start reviewing, there's one thing missing: tests. With an analyzer, it is really important to have a big set of tests, as we don't want to give the wrong suggestions. See https://github.com/exercism/csharp-analyzer/tree/main/test/Exercism.Analyzers.CSharp.IntegrationTests/Solutions/Gigasecond for an example of what a test suite for an analyzer looks like.

Yes, thanks, that was the next step

ErikSchierboom · 2021-11-18T09:08:23Z

Lovely!

Grenkin1988 · 2021-11-18T19:01:10Z

@ErikSchierboom Hi, For some reason I cannot run tests locally, lots of error of missing files.
Could you please take a look at the "comments" PR, this way I can add tests and see them pass here at least.
Thanks

ErikSchierboom · 2021-11-19T07:58:13Z

@Grenkin1988 I'll work on a fix in a separate PR.

N.B. I added f8772de to this PR

ErikSchierboom · 2021-11-19T08:08:36Z

@Grenkin1988 If you rebase on this branch, you should not have the error.

…gration' into weighing-machine-analyzer

…yzer

…n1988/csharp-analyzer into weighing-machine-analyzer

…yzer

Grenkin1988 · 2021-11-25T17:42:58Z

@ErikSchierboom merged everything. Looks it is ready for review

ErikSchierboom

Great work! I've left some minor comments, but it looks impressive!

I noticed that there was no "approved" test case. Could you add one? That way we know that there is a solution that has no comments. Ideally, this should be the exemplar solution, so maybe you could add that as a test case?

src/Exercism.Analyzers.CSharp/Analyzers/WeighingMachine/WeighingMachineComments.cs

src/Exercism.Analyzers.CSharp/Exercism.Analyzers.CSharp.csproj

Grenkin1988 · 2021-11-27T19:08:48Z

Great work! I've left some minor comments, but it looks impressive!

I noticed that there was no "approved" test case. Could you add one? That way we know that there is a solution that has no comments. Ideally, this should be the exemplar solution, so maybe you could add that as a test case?

Done

ErikSchierboom

Lovely. Just some minor comments!

...Exercism.Analyzers.CSharp.IntegrationTests/Exercism.Analyzers.CSharp.IntegrationTests.csproj

ErikSchierboom · 2021-11-30T07:57:28Z

...rcism.Analyzers.CSharp.IntegrationTests/Solutions/WeighingMachine/Approve/WeighingMachine.cs

+    public int Precision
+    {
+        get { return _precision; }
+        private set
+        {
+            _precision = value;
+        }
+    }


I think we'd want people to do:

Suggested change

public int Precision

{

get { return _precision; }

private set

{

_precision = value;

}

}

public int Precision { get; private set; }

It might be good to add a rule for that in the analyzer too.

Rule already exist, looks like test wasn't running before

Do we have a test case that verifies that a comment is made when the student uses:

public int Precision { get { return _precision; } private set { _precision = value; } }

?

It checks that autoproperty is used

https://github.com/exercism/csharp-analyzer/pull/109/files#diff-91639737ce24cbf45cff1d2e6fed7e8ca2ef944bdc33cb5e2549bcb85f3786e2R24

ErikSchierboom · 2021-11-30T09:06:27Z

...Exercism.Analyzers.CSharp.IntegrationTests/Exercism.Analyzers.CSharp.IntegrationTests.csproj

+    <Content Include="@(SolutionFiles)">
+      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
+    </Content>


I think this can be removed.

If this is removed, Cs files are not in VS

Aha. Rider behaves differently then. Okay cool.

ErikSchierboom · 2021-11-30T10:03:14Z

@Grenkin1988 Is exercism/website-copy#2134 up to date with the latest renames?

Grenkin1988 · 2021-11-30T10:04:58Z

@Grenkin1988 Is exercism/website-copy#2134 up to date with the latest renames?

I will check, but it should be

ErikSchierboom · 2021-11-30T10:07:07Z

Okay. I'll do a check on the text.

ErikSchierboom · 2021-11-30T10:12:05Z

I've reviewed the website-copy PR and left some textual comments.

Great work on getting this PR merged! If you found this fun, there are tons more analyzers to implement :)

Grenkin1988 added 4 commits November 13, 2021 20:18

Start writing code

6c63798

Add more analysis for properties

3b18356

Provate field analysis

118bdd7

Add IsRoundMethodCalledInDisplayWeightProperty

b140a1e

ErikSchierboom reviewed Nov 16, 2021

View reviewed changes

Grenkin1988 and others added 2 commits November 16, 2021 23:59

Fix some comments

559a7ab

Merge branch 'exercism:main' into weighing-machine-analyzer

4eea02f

ErikSchierboom reviewed Nov 17, 2021

View reviewed changes

src/Exercism.Analyzers.CSharp/Properties/launchSettings.json Outdated Show resolved Hide resolved

Grenkin1988 added 2 commits November 17, 2021 20:06

Add some comments, and implemet finding correct baking field

895d47d

More comments

41c9b12

Grenkin1988 mentioned this pull request Nov 17, 2021

Add comments for WeighingMachineSolution exercism/website-copy#2134

Merged

Include files and revert launch

6cb3e15

Grenkin1988 marked this pull request as ready for review November 17, 2021 19:26

Grenkin1988 requested a review from a team as a code owner November 17, 2021 19:26

Try adding tests

5c1fb57

Remove explicit solution entries in csproj

f8772de

Remove comments integration from analysis

a0fd8d0

Grenkin1988 added 3 commits November 20, 2021 00:17

Merge remote-tracking branch 'ErikSchierboom/remove-website-copy-inte…

5f43e51

…gration' into weighing-machine-analyzer

Merge remote-tracking branch 'origin/main' into weighing-machine-anal…

8278ab6

…yzer

Merge branch 'weighing-machine-analyzer' of https://github.com/Grenki…

f3716b3

…n1988/csharp-analyzer into weighing-machine-analyzer

Grenkin1988 added 5 commits November 20, 2021 00:27

All tests pass

1e537c7

Auto property case

0cb889d

csharp.weighingmachine.property_setter_is_not_private

0c72335

All tests done and fixed logic

062d381

Merge remote-tracking branch 'origin/main' into weighing-machine-anal…

1216c5c

…yzer

ErikSchierboom reviewed Nov 26, 2021

View reviewed changes

ErikSchierboom added the x:size/large Large amount of work label Nov 26, 2021

Move some comments to shared and add approve case

c780424

ErikSchierboom reviewed Nov 30, 2021

View reviewed changes

Fix project and test

8f9d964

ErikSchierboom reviewed Nov 30, 2021

View reviewed changes

ErikSchierboom approved these changes Nov 30, 2021

View reviewed changes

ErikSchierboom merged commit 543efff into exercism:main Nov 30, 2021

Grenkin1988 deleted the weighing-machine-analyzer branch November 30, 2021 10:40

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Open

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzer for weighing-machine #109

Add analyzer for weighing-machine #109

Grenkin1988 commented Nov 13, 2021

Grenkin1988 commented Nov 13, 2021 •

edited

Loading

ErikSchierboom left a comment

Grenkin1988 commented Nov 16, 2021 •

edited

Loading

ErikSchierboom commented Nov 17, 2021

Grenkin1988 commented Nov 17, 2021

ErikSchierboom commented Nov 18, 2021

Grenkin1988 commented Nov 18, 2021

ErikSchierboom commented Nov 18, 2021

Grenkin1988 commented Nov 18, 2021

ErikSchierboom commented Nov 19, 2021

ErikSchierboom commented Nov 19, 2021

Grenkin1988 commented Nov 25, 2021

ErikSchierboom left a comment

Grenkin1988 commented Nov 27, 2021

ErikSchierboom left a comment

ErikSchierboom Nov 30, 2021

Grenkin1988 Nov 30, 2021

ErikSchierboom Nov 30, 2021

Grenkin1988 Nov 30, 2021

Grenkin1988 Nov 30, 2021

ErikSchierboom Nov 30, 2021

Grenkin1988 Nov 30, 2021

Grenkin1988 Nov 30, 2021

Grenkin1988 Nov 30, 2021

ErikSchierboom Nov 30, 2021

ErikSchierboom commented Nov 30, 2021

Grenkin1988 commented Nov 30, 2021

ErikSchierboom commented Nov 30, 2021

ErikSchierboom commented Nov 30, 2021

Add analyzer for weighing-machine #109

Add analyzer for weighing-machine #109

Conversation

Grenkin1988 commented Nov 13, 2021

Grenkin1988 commented Nov 13, 2021 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

Grenkin1988 commented Nov 16, 2021 • edited Loading

ErikSchierboom commented Nov 17, 2021

Grenkin1988 commented Nov 17, 2021

ErikSchierboom commented Nov 18, 2021

Grenkin1988 commented Nov 18, 2021

ErikSchierboom commented Nov 18, 2021

Grenkin1988 commented Nov 18, 2021

ErikSchierboom commented Nov 19, 2021

ErikSchierboom commented Nov 19, 2021

Grenkin1988 commented Nov 25, 2021

ErikSchierboom left a comment

Choose a reason for hiding this comment

Grenkin1988 commented Nov 27, 2021

ErikSchierboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented Nov 30, 2021

Grenkin1988 commented Nov 30, 2021

ErikSchierboom commented Nov 30, 2021

ErikSchierboom commented Nov 30, 2021

Grenkin1988 commented Nov 13, 2021 •

edited

Loading

Grenkin1988 commented Nov 16, 2021 •

edited

Loading