Top scorers

[ceddlyburge]

Add analyser rules for the Top Scorers concept exercise

Sister PR in webste-copy

Covers the following points from design.md

• filter must be used in removeInsignificantPlayers
• insert must be used in resetPlayerCounts
• sort must not be used in formatPlayers
• updateGoalCountForPlayer and List.foldl must be used in aggregateScorers
• Maybe.map and Maybe.withDefault should be used in formatPlayer
• merge must be used in combineGames

@jiegillet @mpizenberg

[ceddlyburge]

Hi @jiegillet , I think I've answered all of your comments, although I haven't always done what you wanted! Can you take another look! Thanks, Cedd

[ceddlyburge]

There is still one comment you didn't reply to (the exemplar test in failures)

The function is no longer called failures, it's now called tests, so I think this is ok.

The construction rule itself. For example, I just found a Some that should have been a All. This may have been found with a couple of tests.

Yep, this is something that I would think about testing. This was essentially tested originally, but got lost when we merged the two rules in to one. Is merging the two rules in to one definitely the best idea?

Our assumption that the rule even is necessary. That can only be done by coming up with a plausible example

I don't think having a rule that never fires is a problem, and I don't think the test is the right place to work out whether a rule is necessary or not, and having plausible solution increases the workload significantly, which is already beyond our capacity (we still need to write a lot of concept exercises, and nearly none of the practice exercises have any automated rules). I think it would be better to understand this from the usage and the solutions that students submit. A button on the website where a student could state that they thought the automated analysis was wrong would be a useful way of doing this. And a button where a mentor thought that the automated analysis could have picked up a problem but didn't.

[jiegillet]

Yep, this is something that I would think about testing. This was essentially tested originally, but got lost when we merged the two rules in to one. Is merging the two rules in to one definitely the best idea?

Having the rules merged with All or keeping them separate will work just the same, I don't mind either solution.

I think it would be better to understand this from the usage and the solutions that students submit. A button on the website where a student could state that they thought the automated analysis was wrong would be a useful way of doing this. And a button where a mentor thought that the automated analysis could have picked up a problem but didn't.

On the Elixir track we have this comment that gets added whenever there is an analyzer comment. We can do the same, it's not a button, but that's the best we can do right now.

[ceddlyburge]

Hi @jiegillet, so how about I split those combined rules back out in to separate ones and we add the feedback comment that Elixir has.

Is that a workable compromise?

[jiegillet]

Agreed for adding the feedback comment. Probably a different PR would be good though, I'll open an issue.

If you want to split the rule, then I would suggest to split the comments cleanly (one should be about map, and one about withDefault) otherwise you risk raising the same comment twice. But changing Some to All is a lot less work ;)

[ceddlyburge]

Hi @jiegillet , I've added some plausible solutions that fail the analyser, at least semi against my will!

Can you take a look?

Thanks, Cedd

[ceddlyburge]

Hi @jie, I've updated website-copy now that we have finalised the analyser rules, can you take a look?

Thanks, Cedd