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

Experimental feature for unboxing. #122

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

Conversation

ashleyfrieze
Copy link
Contributor

This is what I had in mind for #103.

@greghaskins - does it majorly suck, or does it add value?

@codecov
Copy link

codecov bot commented Sep 4, 2017

Codecov Report

Merging #122 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #122      +/-   ##
===========================================
- Coverage     99.45%   99.3%   -0.16%     
+ Complexity      349     335      -14     
===========================================
  Files            43      41       -2     
  Lines           739     718      -21     
  Branches         22      22              
===========================================
- Hits            735     713      -22     
- Partials          4       5       +1
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/greghaskins/spectrum/Unboxer.java 100% <100%> (ø) 2 <2> (?)
.../internal/configuration/TaggingFilterCriteria.java 96.55% <0%> (-3.45%) 17% <0%> (ø)
...com/greghaskins/spectrum/internal/hooks/Hooks.java 97.29% <0%> (-0.21%) 20% <0%> (-2%)
.../main/java/com/greghaskins/spectrum/Configure.java 100% <0%> (ø) 11% <0%> (-1%) ⬇️
.../java/com/greghaskins/spectrum/internal/Child.java 100% <0%> (ø) 3% <0%> (+1%) ⬆️
...n/java/com/greghaskins/spectrum/internal/Spec.java 100% <0%> (ø) 11% <0%> (-1%) ⬇️
...ectrum/internal/configuration/ConfiguredBlock.java 100% <0%> (ø) 6% <0%> (ø) ⬇️
.../com/greghaskins/spectrum/internal/hooks/Hook.java 100% <0%> (ø) 1% <0%> (-1%) ⬇️
.../java/com/greghaskins/spectrum/internal/Suite.java 100% <0%> (ø) 53% <0%> (-2%) ⬇️
...kins/spectrum/internal/hooks/NonReportingHook.java
... and 3 more

@greghaskins
Copy link
Owner

Nice.

Does it work with concrete types or only interfaces? I'm thinking people would really like to use this for mocks, which are frequently (with Mockito at least) concrete types.

Still mulling over the unbox() entry point. Not the worst thing, but another import/function for people to learn. My first thought was overloading let like let(Something.class, () -> new Something()).

There may also be some sneaky tricks to even avoid passing in that Class object; I feel like maybe Hamcrest or something has some code that just works with generics somehow – unbox(let(() -> new Something()); would be pretty slick.

@ashleyfrieze
Copy link
Contributor Author

The inbox function as provided can be used to surround any supplier, of which both let and junitMixin are such... but we could provide overloads to wrap it further.

It can ONLY work for interfaces. This is a technical constraint to do with dynamic proxies, but also makes sense if you think about how concrete types can have accessible instance variables and multiple constructors, which even Mockito has limitations over.

The type must be specified in the call because the stuff you mentioned about inferring type only works for generic types themselves, not for actual Class objects. In other words, to know at run time the interface to supply, you need a Class object. Even Mockito.mock requires a class object as input.

@ashleyfrieze
Copy link
Contributor Author

My first thought was overloading let like let(Something.class, () -> new Something()).

I've extended my POC to have the equivalent of this - I reverse the order of parameters from the above, because that's what felt natural, but it's easily refactored. I did it for junitMixin and let, but left the unboxer documented and public as this tool could conceivably be used by end-users for their own combinations of things.

How does it look?

@greghaskins
Copy link
Owner

Understood ✅ . A couple thoughts:

  • If it only works with interfaces, we should fail fast (throw an IllegalArgumentException) to help people realize that quickly, rather than getting a stacktrace from deep in the standard library.
  • Since this is experimental, and limited to just interfaces (for now – back of my mind is already wondering how to use ByteBuddy down the road...), let's just use a single entry point like unbox. If this feature takes off and people really like it, we can add the convenience overloads of let and junitMixin.
  • I don't know what the best name for unbox() would be, but I can't think of anything significantly better. unwrap(), proxy(), get(), and from() come to mind; I'm not in love with any of them.

@ashleyfrieze
Copy link
Contributor Author

So, I should remove the overloads of let and junitmixin? It is easy to check if the passed in class is an interface.

Naming the method was hard. It's an auto get or an unwrapper or a de supplier or... nothing sounded good. Accessor?

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.

None yet

2 participants