Misleading foldr tests in python track list-ops

Exercism Support
1

I noticed that the arguments to the lambda are inconsistent with the problem description.

foldr (given a function, a list, and an initial accumulator, fold (reduce) each item into the accumulator from the right using function(item, accumulator));

def test_foldr_direction_dependent_function_applied_to_non_empty_list(self):
        self.assertEqual(foldr(lambda acc, el: el / acc, [1, 2, 3, 4], 24), 9)

The first argument to the lambda should be el, not acc.

The PR that introduced this error is this

When I pointed it out in the PR, as an OSS maintainer myself, I could think of 4 possible ways to respond:

  1. Jeez, error indeed. Thanks for the report, we will get it fixed. Do you want to make a PR for it?
  2. It’s not an error, here’s why.
  3. It’s an error but we are only human and don’t care. There are possibly other errors too.
  4. It’s an error but I’m going to pull out my maintainer hat and lock this thread. Although this is where the error was introduced, I’m going to claim that this is not the place to discuss it.

The 4th option was chosen.

2

This is actually an error in the problem-specifications. All synchronized tracks suffer this inconsistency. The PR fixing this should go to GitHub - exercism/problem-specifications: Shared metadata for exercism exercises., and would most likely be welcome.

The inconsistency might have been introduced when the function parameter order was canonicalized. The text itself is 5 years old.

3

@bruce-wayne - Maintainer here. You commented on an already closed and merged PR. Our current policy is to direct all initial contributor issues/discussions to this forum, so that answers like the one @MatthijsBlom wrote above can be provided - not only for you - but for others reading topics.

This policy is stated in our PR/Issue response messaging (see screengrab below) and in the repo README.

Messaging for PRs and Issues:

image
image919×368 49.7 KB

And as @MatthijsBlom has said, if there is an inconsistency or error, it is upstream in problem-specifications. The Python exercise syncs instructions and test data from there.

4

@bruce-wayne Thank you for opening a forum thread. As I’m sure you’re aware as an OSS maintainer, different projects have different ways of working. Exercism has over 350 OSS repos, dozens of active maintainers and 100 ad hoc ones, and thousands of contributors. We’ve experimented with lots of ways of doing things here over the last decade, and have discovered that the forum works best for us right now.

So while it might feel “incorrect” or even rude to you, please don’t take offence at being redirected to a particular place, and please do respect that the Python maintainer is respecting the rules we have in place org-wide. For more of an understanding of the chaos of our GitHub universe and why we need to do things in certain ways, these are my notifications as of right now…

Screenshot 2023-06-23 at 02.02.37
Screenshot 2023-06-23 at 02.02.37556×970 34.3 KB

On to the issue at hand. @MatthijsBlom @BethanyG (and CC @IsaacG), are we sure that the issue here is in problem-specs?

It seems that the README is fixed (as per the quote from Bruce) but tracks can choose the order of their arguments (they are specified as x/y in problem specs and have the following quote as a comment from Peter Tseng’s:

I understand that some track maintainers may disagree about whether the function argument list should be represented as a string as (acc, el) or (el, acc), but I would like to say to those track maintainers that I think we are just going to have to pick one and run with it. Again, I emphasise that either way there would not be a change to the operation of foldr: The implementing track should be sure to pass the element as el and the accumulator as acc, no matter the string representation here.
I advise implementing tracks to be aware of the conventions common to their language do what makes sense for their language.

So I think two things:

  1. It does seem to be down to the track that implements it to choose the order, and follow the natural conventions of their langauge.
  2. I don’t love that the README in problem specs takes an opinion if (1) is to hold true. I feel like the README should be made more agnostic in problem specs.

Thoughts?

References:

5

I agree that either the order should be fixed by the spec or not mentioned in the spec. Based on past discussions, it does seem as though language have a strong preference regarding the argument order. As such, I agree the specs should not mention an explicit order.

I’m strongly in favor of removing the argument order from the spec (and maybe even going as far as calling out that the the exact argument order needs to be determined by the student or might be called out later in the docs).

6

in favor of removing the argument order from the spec

As a student who has done this exercise in 3 tracks, I would find this very confusing. I’ll argue that for a language that does have native support for foldr (like Haskell), the arguments must follow the order required by the language. For a language that doesn’t have foldr (like Python), why not keep it consistent with the others? The language doesn’t care, so, either way is fine, and only one way is consistent.

7

Python sorta does has a foldl, though. Should foldr be internally consistent with reduce() or with the specs? If only things were so black and white.

8

edit: sorry, wasn’t meant as a reply to a specific message.

Please let us stay on-topic. This thread is about documentation and (indirectly) canonical data design, not about foldr.

(I’m fine with discussing foldr in another thread. (Again.) I have plenty to say on the matter.)

I agree with @IsaacG’s points, and would add (hopefully to @bruce-wayne’s reassurance) that parameter order can be specified in instruction.append.mds, and that we could decide to strongly suggest or even require this.

10

I agree - problem specs should not mention explicit order in either the cannonical data nor the README, and tracks should implement this exercise based on their language conventions and expectations.

Edited to add

While it is off topic for this thread, I do want to mention here that this exercise on the Python track has flipped back and forth in argument order several times. The last time this happened, there was considerable debate that drained both my time and energy as a maintainer. I don’t think there is a “right” answer to that debate, and I suspect reverting the current order will simply lead to another debate down the road when the next passionate student finds fault with it.

This exercise has had multiple questions and issues, and is in need of some concentrated TLC. I was irritated enough with the last debate I didn’t enforce creating a detailed instruction append. Suffice to say it is now on my to-do list.

11

Proposal: drop fold argument ordering from the specs:

1 Like
12

This PR has been merged to the problem specs repo. This change should show up in track repos when they sync this exercise.

1 Like
13

FWIW, the issue was only in the description.md. We canonical tests were already edited (I worked on it in the past) to change the commentary to say that the track should use whatever order makes sense for the language.

I think the PR that was (just) merged was the right way forward :slight_smile:

1 Like