Fix(accumulate): remove extra ) in canonical-data.json

[ynfle]

Fixes: #1920

[ynfle]

I have to read the docs before I mutate the tests 🤦‍♂️

[SaschaMann]

The exercise is deprecated: #1918. Not sure if that means that this shouldn't be merged, though.

[ynfle]

The exercise is deprecated: #1918. Not sure if that means that this shouldn't be merged, though.

Is there an easy way to see if any tracks still have it without having to download every tracks' git repo?

[ynfle]

@SaschaMann after a quick url test the following languages have accumulate available as an exercise:

• python
• javascript
• sharp
• haskell
• clojure
• sharp
• ruby
• java
• elixir
• elm
• erlang
• go
• julia
• kotlin
• php
• perl5
• rust
• swift
• scala
• typescript
• wren

[petertseng]

I think we would merge fixes to existing tests. I think we would advise against adding new tests.

[ynfle]

For a complete list and analysis of how many added "status": "deprecated", the following bash script with jq:

for i in `curl -s https://exercism.org/api/v2/tracks | jq -r '.tracks[].slug'`
do
	echo "$i": `curl -s https://raw.githubusercontent.com/exercism/"$i"/main/config.json | jq '.exercises.practice[] | select(.slug=="accumulate").status'`
done | grep 'null\|deprecated'

yielded

csharp: null
clojure: null
coffeescript: null
elixir: "deprecated"
elm: null
erlang: null
fsharp: null
go: "deprecated"
groovy: null
haskell: null
java: "deprecated"
javascript: "deprecated"
julia: "deprecated"
kotlin: "deprecated"
lfe: "deprecated"
lua: "deprecated"
perl5: null
php: null
purescript: null
python: "deprecated"
racket: "deprecated"
raku: null
reasonml: null
ruby: null
rust: null
scala: null
scheme: null
sml: null
swift: null
tcl: null
typescript: null
vbnet: null
wren: null

where null means implemented but no status key & "deprecated" means implemented and status key.

Results:

Deprecated	Count
true	10
false	23
Total	33

EDIT: Made bash script runnable

[junedev]

@ynfle Just FYI The exercise was officially deprecated 5 days ago and there was no notification to tracks yet so your analysis does not give the picture of how many would deprecate if they knew about it.

[ynfle]

@ynfle Just FYI The exercise was officially deprecated 5 days ago and there was no notification to tracks yet so your analysis does not give the picture of how many would deprecate if they knew about it.

True. I don't think any amount of bash and jq could help with that yet...

[ee7]

I think we would merge fixes to existing tests. I think we would advise against adding new tests.

I agree. I think the original commit b3e2f50 has the better approach here.

The value of this accumulator key before this PR is just wrong; I have to imagine that every track that implements this test case has implemented it identically to how they'd implement the typo-free version. So I'd argue that we should treat this like we treat fixing a typo in a description - just mutate it when it doesn't change the "meaning". See #1687 for the discussion that led to description becoming mutable in some circumstances, and #1709 for discussion of error becoming mutable.

Adding a new test case means that every implementing track has a noisy update to the corresponding tests.toml file without changing their user-facing tests file. We might think that's less painful given that this exercise is deprecated, and so tracks might feel less need to update. But configlet sync currently exits non-zero when there is only an update to a deprecated exercise (regardless of whether it's deprecated only in prob-specs, only on the track, or both). That is, a track is currently incentivized to update a deprecated exercise if they want an exit status of 0 from a track-wide configlet sync.

If we just mutate then also note that:

• CI will fail (see https://github.com/exercism/problem-specifications/runs/4846996760) at merge-time. That's probably what we want, unless we add some logic like "ignore mutation if commit/PR comment contains special phrase". I wouldn't support something like "allow mutation if the diff is only one character" - a one-char diff could be a "bug fix" that meaningfully changes the test, and so merits a new reimplements test.
• We should update https://github.com/exercism/problem-specifications#test-data-canonical-datajson to reflect what we want.

[SaschaMann]

just mutate it when it doesn't change the "meaning".

While in this case it may be quite clear, there are a lot of different ideas of what changes the meaning and what is only a bug fix which leads to pointless and endless discussions. The requirement that test cases are immutable even if they contain a bug is there for a reason.

[ynfle]

Either way, I think everyone can agree its a typo here. @iHiD thoughts? (I feel bad pinging you, I'm sure there are lots of things going on)

[ErikSchierboom]

First off, the fact that we've deprecated an exercise does not mean that tracks must deprecate the exercise, only that we recommend they do so. There can be very valid reasons why a track would want to keep a deprecated exercise. I do admit that it should be the exception though.

Taking that into account (that tracks might keep the exercise active), whether or not we should merge fixes then becomes easy to answer: we should.

As to the discussion on whether a we need a new test case, I'm of the opinion that we do. Yes, it is rather clearly a simple typo, but tracks could have their generator depend on that typo, which means that fixing it would break those generators. It is precisely to avoid these scenarios that we introduced the whole immutability property. I really don't want to have a situation where we'll have to decide collectively whether or not something is a bug fix.