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

Add Ops #95

Draft
wants to merge 1 commit into
base: v1
Choose a base branch
from
Draft

Add Ops #95

wants to merge 1 commit into from

Conversation

htmldoug
Copy link
Contributor

@htmldoug htmldoug commented Oct 13, 2021

This is a first step toward open sourcing some of our higher-level weePickle code, starting with ConcatFromInputs. The big decision is where it should live. We could carve out another jar, but I think it makes the sense to add most of the visitation stuff directly to weepickle-core.

@htmldoug htmldoug marked this pull request as ready for review October 13, 2021 22:22
object WeePickle extends LowPriorityImplicits {
object WeePickle
extends LowPriorityImplicits
with FromInputOpsImplicits {
Copy link
Contributor Author

@htmldoug htmldoug Oct 13, 2021

Choose a reason for hiding this comment

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

In the future, I expect to add FromOpsImplicits/ToOpsImplicits/VisitorOpsImplicits for .tolerant, etc. Grouping the extensions into a "high level extension ops" traits seems like it should help discoverability.

Copy link

Choose a reason for hiding this comment

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

I think this would be a good thing to add to the code, it explains your reasoning which is often hard to "reverse engineer" from the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

also BufferedValueOps?

@htmldoug htmldoug force-pushed the ops branch 2 times, most recently from 6e90bd9 to 18cf0a7 Compare October 13, 2021 22:45
@arkban
Copy link

arkban commented Oct 14, 2021

The big decision is where it should live. We could carve out another jar, but I think it makes the sense to add most of the visitation stuff directly to weepickle-core.

+1 to put it in with the rest of the stuff. Having to manage multiple JAR versions is always a little error prone.

object WeePickle extends LowPriorityImplicits {
object WeePickle
extends LowPriorityImplicits
with FromInputOpsImplicits {
Copy link

Choose a reason for hiding this comment

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

I think this would be a good thing to add to the code, it explains your reasoning which is often hard to "reverse engineer" from the code.

@htmldoug htmldoug marked this pull request as draft October 15, 2021 19:26
object WeePickle extends LowPriorityImplicits {
object WeePickle
extends LowPriorityImplicits
with FromInputOpsImplicits {
Copy link
Contributor

Choose a reason for hiding this comment

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

also BufferedValueOps?

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.

4 participants