selfrefactor / rambdax

Extended version of Rambda
https://selfrefactor.github.io/rambdax
MIT License
221 stars 26 forks source link

Index on map breaks currying #57

Closed Boudewijn26 closed 4 years ago

Boudewijn26 commented 4 years ago

I might be missing something, but I ran into the issue that map calls the provided function with both the element and the index. This prevents me from doing something like

const isWeb = R.piped(["http://", "https://"], R.map(R.startsWith), R.anyPass);
const result = isWeb("http://example.com");

As R.startsWith will receive "http://", 0. And try to call 0.startsWith.

Of course I can work around:

const isWeb = R.piped(["http://", "https://"], R.map(R.pipe(R.always, R.startsWith)), R.anyPass);
const result = isWeb("http://example.com");

But I lose some of the satisfaction I normally get from this kind of functional programming.

Do you have suggestions or better workarounds?

selfrefactor commented 4 years ago

Maybe I have to stop passing index as second argument. This is valid not only for R.map but for R.filter as well. This will be fixed within 2 days and a new version will be released.

Boudewijn26 commented 4 years ago

Thanks for the quick response. I think you can also include reduce into that list.

Seeing as how this would be a breaking change, I can imagine for the fix an inclusion of mapUnindexed and friends would be preferable over changing the existing functions. Then the next major version could apply the fix to map and friends while perhaps including a mapIndexed?

selfrefactor commented 4 years ago

Yeah, this is breaking change for both libraries - Rambda and Rambdax. This issue create 2 major versions :)

There are more methods that do that, so the actual fix will take a bit longer than previously stated; still it will happen this week.

As for reduce - if Ramda does not pass the index, then I will do the same.

As for keeping the current logic as new methods - this seems like the perfect solution. It also means that about 6, 7 new methods will arrive in Rambdax.

selfrefactor commented 4 years ago

Actually, we have a misunderstanding as this works just fine:

const result = R.pipe(
  R.map(R.startsWith('https')),
)(["http://", "https://"])

In other words passing index doesn't brake your example, rather than your understanding of how R.piped works. It doesn't return a function, but it offers the convenience to see your input right before your first pipe.

You can check also https://remedajs.com/docs#pipe as Rambdax.piped has the same API. I will close this issue, but feel free to comment further.

Boudewijn26 commented 4 years ago

You appear to have misunderstood my initial snippet. I'll write it out:

const isHttp = R.startsWith("http://");
const isHttps = R.startsWith("https://);
const isWeb = R.anyPass([isHttp, isHttps]);

// Deduplicate the first two lines:
const protocols = ["http://", "https://"];
const isHttpList = R.map((a) => R.startsWith(a), protocols);
const isWeb = R.anyPass(isHttpList);

// Use piped instead of explicitly passing arguments
const isWeb = R.piped(
  ["http://", "https://"],
  R.map((a) => R.startsWith(a)),
  R.anyPass
);

// All of these isWebs can be called
const notWeb = isWeb("file://local");
const web = isWeb("https://google.com");

The issue here is that changing R.map((a) => R.startsWith(a)) to R.map(R.startsWith) breaks, because of the index passed. It turns into R.map((str, index) => R.startsWith(str, index)). R.startsWith then calls startsWith on the index argument, which errors out.

selfrefactor commented 4 years ago

Your more detailed example helped. I will reopen the issue until we reach a consensus.

I would prefer to see the whole example done with Ramda.pipe and Ramda methods, just to show the logic you try to accomplish. Now, I still cannot fully see what is your goal. In my previous commend I gave R.pipe example as this was the use case that worries me more than your example.

Boudewijn26 commented 4 years ago

My original usecase was checking if a currently checked out git branch either started with a JIRA issue: eg. ABC-01 or started with feature/ABC-01. Since I found this was rather specific, I thought a check to see if a URL starts with a certain protocol would be a more familiar usecase.

If I were to only use Rambdax.pipe and other Rambdax methods, you'd get something like:

const isWeb = R.pipe(
  R.map((a) => R.startsWith(a)),
  R.anyPass
)(["http://", "https://"]);
const result = isWeb("http://example.com");

Or even inlining the isWeb

const result = R.pipe(
  R.map((a) => R.startsWith(a)),
  R.anyPass
)(["http://", "https://"])("http://example.com");

You could add another pipe here which doesn't do anything

const result = R.pipe(
  R.pipe(
    R.map((a) => R.startsWith(a)),
    R.anyPass
  )(["http://", "https://"])
)("http://example.com");

All of these are equivalent. The plain ol' javascript equivalent would be

const isWeb = (url) => url.startsWith("http://") || url.startsWith("https://");
const result = isWeb("http://example.com");

In your example

const result = R.pipe(
 R.map(R.startsWith('https')),
)(["http://", "https://"])

You are using map to pass the second argument to R.startsWith, so you get [R.startsWith('https', 'http://'), R.startsWith('https', 'https://')] which of course would be [false, true]. I want to use map to curry R.startsWith, so you get [R.startsWith("http://"), R.startsWith("https://")], which has no definitive value. But I can use this as a list of predicates to pass into R.anyPass: R.anyPass([R.startsWith("http://"), R.startsWith("https://")]).

I hope this clarifies my intentions for you.

selfrefactor commented 4 years ago

Yes, I finally managed to fully see your intentions. I admit that passing index as additional argument to R.map brakes the example. Still, for now I will leave it as it is, but I will leave this issue open, because I do intend on one point to fix it. Maybe I will wait for more braking changes to materialize. Again, thank you for bringing it up.

Boudewijn26 commented 4 years ago

I would be willing to put in the time to fix this. Would you accept such a pull request or would you prefer to wait before introducing such a breaking change?

selfrefactor commented 4 years ago

Thank you for the suggestion, but I will wait a bit to see if there are upcoming braking changes. As for PR - it is mostly deletion of code and tests which is not hard to do. When I fix the issue, the current logic won't stay as new methods such as R.mapIndexed as we previously discussed.

As timeframe - I think that one month will be sufficient for the fix to be applied.

selfrefactor commented 4 years ago

It should be fixed with latest 6.0.0 version. Closing now, but feel free to reopen if the issue is not solved properly.

selfrefactor commented 4 years ago

Additional note:

Now this is added as difference between Ramda and Rambda:

- Rambda's **map**, **filter**, **partition** when they iterate over objects, they pass property and input object as predicate's argument.

In other words, while I changed how these methods behave when they have arrays as input, there is no change when the iterable is object.