Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement folds for references #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ImmemorConsultrixContrarie
Copy link
Contributor

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented May 20, 2021

Closes #180.

Things to do before this draft PR could be made an actual PR:

  • Implementation
  • Tests
  • Bikeshedding inherent methods. Do we want to completely remove those and force user to import fold traits (maybe like use frunk::prelude::*;)? Do we want to have something like foldl_by_ref, foldl_by_mut, foldl_by_val?
  • Adjust documentation according to what was decided in the bikeshedding part

Bikeshedding and doc changes will be part of another PR: #181 (comment)

@ImmemorConsultrixContrarie
Copy link
Contributor Author

Funny fact: inherent foldl method does not interfere with (&hlist).foldl or (&mut hlist).foldl calls.
However, I still think that we should remove or rename the inherent methods; due to the same names for methods in struct and in trait, compiler could emit some misleading errors

@lloydmeta
Copy link
Owner

Since there's no conflict, I would prefer that we keep this PR to just implementing the fold methods for references.

IMO, if the user is using the "wrong" folder "function" with a given binding (e.g. hlist value but folder expects references), then they should get an error, which is the same as if you pass a reference-expecting closure when .maping over a vec.into_iter(): there is no separate map_val or map_mut.

@ExpHP
Copy link
Collaborator

ExpHP commented May 21, 2021

However, I still think that we should remove or rename the inherent methods; due to the same names for methods in struct and in trait, compiler could emit some misleading errors

I'm not at all keen on removing them, as I feel this can hurt discoverability. Having them there gives us this; the whole API of HLists documented on a single page, in precisely the form that we are used to consuming documentation in as rustaceans. Prior to 2.0, the closest you could get was to look at examples in README.md or on the hlist module—or to look at the traits in the docs, which were scary.

I would go so far as to say I'd rather have three inherent methods (foldl, foldl_ref, foldl_mut), and if I were to rename anything I'd rename the trait method to either foldl_ or foldl_generic. But this could be a slippery slope; there are other methods that could use _ref and _mut variants, yet obviously it'd be crazy to add these variants for all methods. (probably just maps and folds?). But personally this is why I've been satisfied with just having orthogonal to_ref()/to_mut() methods.

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie marked this pull request as ready for review May 22, 2021 13:11
@redbaron
Copy link

Been hit by the lack of folding on reference. I know it's been a while, but maybe there is a way forward to merge it? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Folds are not implemented for &hlist and &mut hlist
4 participants