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

De-duplicate ValueClassSupport #913

Merged
merged 13 commits into from Sep 6, 2022

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Sep 4, 2022

  • Create new module for platform implementations (there was no existing suitable project, due to the internal dependencies between them),
  • move ValueClassSupport into it (deleting the duplicated versions in Android and DSL modules)

Fixes #857

Are there any other classes that can be de-duplicated?

…pport into it (deleting the duplicated versions in Android and DSL modules)
@aSemy aSemy marked this pull request as ready for review September 4, 2022 12:03
@aSemy
Copy link
Contributor Author

aSemy commented Sep 4, 2022

@qoomon Can you take a look? What do you think?

@Raibaz
Copy link
Collaborator

Raibaz commented Sep 5, 2022

LGTM, waiting for a check by @qoomon as well.

Thanks!

@qoomon
Copy link
Contributor

qoomon commented Sep 6, 2022

Sorry for the late response, I'll do the review now.

Copy link
Contributor

@qoomon qoomon left a comment

Choose a reason for hiding this comment

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

LGTM, however I think the wording platform is a little misleading.

Is there a reason why don't implement ValueClassSupportwithin the common mockk module?
https://github.com/aSemy/mockk/tree/fix/de-duplicate-valueclasssupport/modules/mockk/src/commonMain/kotlin/io/mockk

@aSemy
Copy link
Contributor Author

aSemy commented Sep 6, 2022

LGTM, however I think the wording platform is a little misleading.

Yeah I think the name could be better. mockk-core, mockk-common, mockk-shared-tools, mockk-tools?

Is there a reason why don't implement ValueClassSupportwithin the common mockk module? https://github.com/aSemy/mockk/tree/fix/de-duplicate-valueclasssupport/modules/mockk/src/commonMain/kotlin/io/mockk

Because mockk depends on mockk-android and mockk-dsl

https://github.com/aSemy/mockk/blob/df2ccd4f3f8575554c3083b957bc3b0e36827c98/modules/mockk/build.gradle.kts#L22-L25

it can't be depended on by the other modules.

If I were to refactor the project then I would have the root project be io.mockk:mockk, and it would depend on the subprojects as API dependencies and not have any code. Then I'd rename io.mockk:mockk to io.mockk:mockk-core, and that would be the shared place for code. mockk-android would be an extension of mockk-core.

@qoomon
Copy link
Contributor

qoomon commented Sep 6, 2022

Thanks for the explanation. Regarding the module name I'd vote for mockk-core.

@Raibaz
Copy link
Collaborator

Raibaz commented Sep 6, 2022

+1 for mockk-core.

@aSemy aSemy force-pushed the fix/de-duplicate-valueclasssupport branch from 2a26d49 to 1badf45 Compare September 6, 2022 15:43
- remove InternalPlatformDsl.unboxClass util (it's the same as ValueClassSupport.boxedClass)
- move InternalPlatformDsl.boxCast to ValueClassSupport
…pecifically for value classes, so it makes more sense to be in a general util class)
@aSemy
Copy link
Contributor Author

aSemy commented Sep 6, 2022

Okay, renamed! I changed the package too.

I also removed fun unboxClass(cls: KClass<*>): KClass<*> from InternalPlatformDsl, since it was just a re-name of the ValueClassSupport.boxedClass util.

@Raibaz
Copy link
Collaborator

Raibaz commented Sep 6, 2022

LGTM

@qoomon ?

@qoomon
Copy link
Contributor

qoomon commented Sep 6, 2022

Love it 🙂

@qoomon
Copy link
Contributor

qoomon commented Sep 6, 2022

@aSemy thanks for your effort

@Raibaz Raibaz merged commit 53f22ed into mockk:master Sep 6, 2022
aSemy added a commit to aSemy/mockk that referenced this pull request Sep 16, 2022
@aSemy aSemy mentioned this pull request Jan 15, 2023
3 tasks
@aSemy aSemy deleted the fix/de-duplicate-valueclasssupport branch January 15, 2023 23:33
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.

Reduce code duplication across subprojects
3 participants