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

Permit mocking stackable trait pattern in scala 3 #528

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

Conversation

hughsimpson
Copy link
Contributor

Pull Request Checklist

  • I agree to licence my contributions under the MIT licence
  • I have added copyright headers to new files
  • I have added tests for any changed functionality

Fixes

Fixes mocking classes that implement the stackable trait pattern (i.e. abstract override def) in scala 3

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Aug 1, 2024

Maybe not the forum to say this, but other than #527 this was the only bug I've hit so far on migrating from scala 2 to scala 3, so absolutely incredible job on compatibility here.

@barkhorn barkhorn added this to the v6.1.0 milestone Aug 2, 2024
@hughsimpson
Copy link
Contributor Author

hughsimpson commented Aug 7, 2024

I appreciate there's a nuance here of 'do we really want to make it impossible to mock artifact methods?'. I think they're already essentially not unlockable? As in it would probably be pointless to try to use them in any mock. So I'm pretty sure that it's a legit forbid

@goshacodes
Copy link
Contributor

goshacodes commented Sep 14, 2024

In my opinion, we shouldn't allow mocking this.
The only things that should be mocked are traits or abstract classes not extending anything, because if you mock something else you are breaking Interface segregation principle and this leads you the wrong way.
But now we allowing to mock a lot of things, e.g. classes, so this can be legit

@hughsimpson
Copy link
Contributor Author

@goshacodes sorry for taking so long to get to this, pushed the changes -- hadn't realised it was as simple as adding a new case to the MockableDefinitions constructor, nice catch. Added a couple more tests, too.

@goshacodes
Copy link
Contributor

@barkhorn let's merge it

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

Successfully merging this pull request may close these issues.

3 participants