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

Make offset an argument of overdub_pass! #168

Closed
wants to merge 2 commits into from
Closed

Make offset an argument of overdub_pass! #168

wants to merge 2 commits into from

Conversation

MasonProtter
Copy link

Given the hesitance to merge #157, I'd like to implement ReflectOn in a separate package. However, overdub_pass! is a gigantic function and in order to make ReflectOn work, I'd need to copy paste all of overdub_pass! just so I can add offset=2 for the ReflectOn method. Instead, I think it'd be nice if offset could just be an argument to overdub_pass!. I think this would be beneficial to others who might want to tweak or alter overdub as well in a similar way.

@vchuravy
Copy link
Member

I am less opposed of this since it is relatively small. What would actually be rather interesting is to take Shashi's ReflectOn idea and use it to implement Cassette's Tagging as an extension.

@MasonProtter
Copy link
Author

I don't know much about tagging, but naïvely, it seems like it's a better candidate for a separate package than ReflectOn is.

RelfectOn seems like an almost quintessential cassette action. Take a method call and force it to go to a different method that may be overly restrictive.

@MasonProtter
Copy link
Author

I'm not actually sure yet if this PR would be sufficient to get ReflectOn working in a separate package. I think there's an issue with how overdub is defined blocking my strategy from working, so merging this may be pointless.

@vchuravy
Copy link
Member

RelfectOn seems like an almost quintessential cassette action. Take a method call and force it to go to a different method that may be overly restrictive.

Right my reservation is that tagging and ReflectOn are supposed to do pretty much the same thing, but they do it in parallel. I would be happy to make tagging less of a core Cassette thing.

@MasonProtter
Copy link
Author

@shashi any thoughts?

@MasonProtter
Copy link
Author

Right my reservation is that tagging and ReflectOn are supposed to do pretty much the same thing, but they do it in parallel. I would be happy to make tagging less of a core Cassette thing.

Tagging can only do the same thing as something like RelfectOn if you have an actual value to pass around. If you don't have a legit value to tag, ReflectOn is more general.

@shashi
Copy link

shashi commented Feb 20, 2020

Yeah I think in theory we can add ReflectOn here and move the Tagging stuff out into another package and have it rely on ReflectOn. I'm not a 100% sure that that wouldn't need more features from Cassette.

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.

3 participants