markrogoyski / itertools-php

PHP Iteration Tools Library
MIT License
140 stars 11 forks source link

Method `retro()` added to `Single` and `Stream` namespaces #28

Closed Smoren closed 1 year ago

Smoren commented 1 year ago

Hi @markrogoyski,

I've added methods for reversing iterables.

Summary:

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4153105360

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 4153090303: 0.002%
Covered Lines: 633
Relevant Lines: 634

💛 - Coveralls
Smoren commented 1 year ago

I've rebased this branch from develop.

markrogoyski commented 1 year ago

Hi @Smoren,

Thank you for the PR with the new functionality.

I think you probably want to call this reverse rather than retro. Retro means like old fashions from the past.

Smoren commented 1 year ago

Hi @markrogoyski,

I named the same as a similar function in D language.

But if you think that the name is not suitable, I will rename it.

markrogoyski commented 1 year ago

Hi @Smoren,

Names are hard, and often subjective. There is no single correct answer, only tradeoffs.

When thinking about a good name, I like to consider the intuitive discovery of the user, and prior art.

For something like reversing a list, I think most people would be searching for the word "reverse." Upon seeing a function called reverse, it would be clear what it does from the name alone. The name "reverse" leads to users discovering the functionality on their own. "Retro," on the other hand, is not intuitive, and it is unlikely that someone would be searching for that keyword for reverse functionality. Retro, in English, usually refers to older fashion made popular again, or an end-of-sprint meeting to discuss the previous work period (short for retrospective).

The other thing is prior art. Most programming languages would call this functionality reverse. Even D has a method called reverse. Therefore, I think both intuitive discovery and prior art point to "reverse" being the appropriate name here.

In your other PR, tee on the other hand I think would favor prior art over discoverability. Tee is common enough in the Linux/Unix CLI and some programming languages, that even though it may not be the intuitive name a less-experienced programmer would search for, it is the established name for duplicating input to multiple targets. While something like duplicate might be what someone may search for, tee is the established name here, and I would go with tee for that function. Another example, if someone didn't know what a regular expression was, they might search for "pattern matching" or something, but regex is the established name for this, so prior art wins.

Smoren commented 1 year ago

Hi @markrogoyski,

Agree with you. I'll rename this method when I get access to my PC.

Smoren commented 1 year ago

Hi @markrogoyski,

I've renamed retro() to reverse().

BTW, what do you think about making a new interface for iterators named BidirectionalIterator and reduce reverse() memory usage for it's instances?

Smoren commented 1 year ago

Hi @markrogoyski,

I've rebase this branch from develop and resolved the confilcts.

Smoren commented 1 year ago

This branch is rebased from develop, conflicts are resoved.

markrogoyski commented 1 year ago

Thanks for adding reverse and iterating on the naming.

Smoren commented 1 year ago

Thanks Mark for merging this PR!