Skip to content
New issue

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

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

Already on GitHub? # to your account

Foldable operations on Project #68

Merged

Conversation

pepegar
Copy link
Member

@pepegar pepegar commented Aug 31, 2018

This PR:

@pepegar pepegar requested a review from andyscott September 4, 2018 07:17
Copy link
Member

@andyscott andyscott left a comment

Choose a reason for hiding this comment

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

This is pretty great! I'm 👍 for the tagged types as you've implemented them. I have a few notes/discussion items regarding organization before we merge.

Copy link
Member

@andyscott andyscott left a comment

Choose a reason for hiding this comment

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

Alright! Prepare for a brain dump of some of my overall thoughts on Droste's design 😄:

My biggest ask (or complaint?) is that I'd like all functions in Droste to delegate to "static" functions on helper objects or in package objects that perform the specific operation. These static methods should always exist-- that is to say that you don't need to summon an implicit value and call the method on said value. Methods on implicit values are allowed, but they must statically delegate to an identically named method. This is absolutely a design decision on my part, and is perhaps worth a proper discussion in a separate issue.

In this case, for example, I'd like to see some kind of foldable helper object that has all, any, collect, contains, etc implementations. The functions on the Project type class would simply delegate to these static functions. It's boilerplate, which is ugly, but I like having all core functionality implemented in the most straightforward and least magical way possible.

If you need this functionality ASAP, feel free to merge as is-- the value of this new functionality outweighs blocking the PR.

@pepegar
Copy link
Member Author

pepegar commented Sep 6, 2018

I like the idea @andyscott. I'll update the PR to be able to merge soon :)

@pepegar pepegar force-pushed the foldable-operations-on-project branch from 7423af9 to c328ae1 Compare September 6, 2018 19:00
@pepegar
Copy link
Member Author

pepegar commented Sep 6, 2018

OK, so I've ended up moving de implementation of those functions to the Project companion, since that's where I think it belongs (even though they're like traverse operations), and because I prefer not to clutter the project with top level objects ;)

@andyscott
Copy link
Member

Awesome!

@pepegar pepegar merged commit c8a065a into higherkindness:master Sep 7, 2018
@pepegar pepegar deleted the foldable-operations-on-project branch September 7, 2018 07:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants