Skip to content

Adding traverse to List, Option, Either and Tuple2#80

Closed
amarpotghan wants to merge 24 commits into
functionaljava:masterfrom
amarpotghan:master
Closed

Adding traverse to List, Option, Either and Tuple2#80
amarpotghan wants to merge 24 commits into
functionaljava:masterfrom
amarpotghan:master

Conversation

@amarpotghan

Copy link
Copy Markdown
Contributor

No description provided.

@tonymorris

Copy link
Copy Markdown
Contributor

FWIW, this is specifically List#traverse. You might also consider writing traverse for every possible Traversable. Unfortunately, that is a lot of work, since there are infinity of them, so as a compromise, perhaps the more common ones; List, Option, Either<T, _>, Tuple2<A, _>

@amarpotghan

Copy link
Copy Markdown
Contributor Author

@tonymorris Oops...i totally did not see that. I agree that it's a list traverse.
I was planning to wtite it for every traversable, but as u said its a lot of work. That's why decided this specific one...because i found myself writing this a couple of times. I will update the PR.
Thanks.

@amarpotghan amarpotghan reopened this Jan 12, 2015
@tonymorris

Copy link
Copy Markdown
Contributor

Yeah no worries. You'll probably want the others some day too!

@amarpotghan

Copy link
Copy Markdown
Contributor Author

@tonymorris sure thing 👍

@amarpotghan amarpotghan changed the title Adding traverse to Option type. Adding traverse to List, Option, Either and Tuple. Jan 12, 2015
@amarpotghan amarpotghan changed the title Adding traverse to List, Option, Either and Tuple. Adding traverse to List, Option, Either and Tuple2 Jan 12, 2015
@mperry

mperry commented Jan 12, 2015

Copy link
Copy Markdown
Contributor

@tonymorris and @amarpotghan
For some reason I thought traverse was defined on Monad with the signature:
traverse:: Monad f => List a -> (a -> m b ) -> m (t b)
whereas it is actually on Traversable:
traverse :: Applicative f => (a -> f b) -> t a -> f (t b)

Given the use of different applicatives, is the best way to handle this to represent the function on a traversable type t as:
public <B> ${Applicative}<${Traversable}<B>> traverse${Applicative}(F<A, ${Applicative}<B>>)
so for this example by @amarpotghan for traversing lists we would have on class List:
public <B> Option<List<B>> traverseOption(F<A, Option<B>)

Some other classes this would apply to that I use: IO(?), SafeIO, Seq, Set(?), Stream, P1, Validation<A, _>, State<S, _>.

This of course is starting to create alot of combinations, but it would be good to agree on the approach and fill in the missing implementations at some future point.

@tonymorris

Copy link
Copy Markdown
Contributor

Yep that's the problem. You have NxM instances to write where:

  • N = Number of Applicative instances
  • M = Number of Traversable instances

Given the limitations of Java's type system against the intractability of this problem, Functional Java has had a history of, "just pick the most common ones that I have the energy to write today."

@tonymorris

Copy link
Copy Markdown
Contributor

Consequently, I cannot speak for the energy of @amarpotghan.

@mperry

mperry commented Jan 14, 2015

Copy link
Copy Markdown
Contributor

For TODO things, we should create Github issues. We can decide what we want to do for traverse here and then create an issue documenting what is left to do.

I had a look at doing this for the Option type last night (so don't worry about Option). BTW, I also had a look at the laws that traversal had to comply with, but only really understood the identity law. I suspect this won't be a barrier to writing the code.

@amarpotghan

Copy link
Copy Markdown
Contributor Author

@mperry Thanks. I will not touch Option type.

@mperry

mperry commented Jan 20, 2015

Copy link
Copy Markdown
Contributor

No problem. FYI, I started reviewing these changes, but have not had time to finish yet. I will try to do some more tonight.

@amarpotghan

Copy link
Copy Markdown
Contributor Author

@mperry thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented above that you were traversing over the left value and here you are traversing over the right value. I did not think you would do both. Is there a conflict here for the compiler in calls to traverseList(z -> ...)? Should this be renamed to traverseLeftList/traverseRightList?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mperry both resides on two different classes, i.e RightProjection and LeftProjection. client has to specify which projection he has to traverse, e.g e.right().traverseList(z -> ...). I think we should have traverseLeftList and traverseRightList in Either class.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added translators for other applicatives too. Just worried if we really want to provide this convenience. I mean e.right().traverseList(z -> ...) is almost as good as e.traverseListRight(z -> ..)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, from the diffs I did not realise this was on the left and right projection. This is fine as is then.

Comment thread core/src/main/java/fj/data/List.java Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to rename these variables, e.g. on line 372 as represent Option<List<B>>. Why not use obs or olb here? Same with the map2Option method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!, you will see that changed soon!

@mperry

mperry commented Jan 21, 2015

Copy link
Copy Markdown
Contributor

I have had enough for now. Will check back tomorrow. Thanks for your hard work, it is much appreciated.

@amarpotghan

Copy link
Copy Markdown
Contributor Author

screen shot 2015-01-21 at 8 31 38 pm
@mperry Thanks for all the observations. I changed the required, please have a look :). In the validation class (without type annotations), couldn't guarantee Intellij support. I hope that works in Intellij 14. My side, I will upgrade soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the hamcrest dependency when we are just saying the two lists are equal? I can't find a dependency to hamcrest currently. Am I missing something or is this a new dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junit has it since a long ago

@mperry

mperry commented Jan 22, 2015

Copy link
Copy Markdown
Contributor

I think we are almost done. Just the hamcrest issue and either javadoc to address.

@mperry

mperry commented Jan 22, 2015

Copy link
Copy Markdown
Contributor

Still places in the javadoc that mention fail, particularly for Either. Go to the "files changed" tab in this PR and search for "fail". We are almost there...

mperry added a commit that referenced this pull request Jan 23, 2015
@mperry mperry added this to the v4.3 milestone Jan 23, 2015
@mperry mperry closed this Jan 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants