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

Expose less implementation details in Local #946

Merged
merged 2 commits into from Jul 19, 2019

Conversation

@Avasil
Copy link
Collaborator

commented Jul 8, 2019

My take on #923

Just added few privates.
I've left Context.mkIsolated and Context.bind public because they are used by TaskLocal

@Avasil Avasil requested review from alexandru and oleg-py Jul 8, 2019

@@ -114,7 +114,7 @@ object Local {
}

/** Macros implementations for [[bind]] and [[bindClear]]. */
class Macros(override val c: whitebox.Context) extends InlineMacros with HygieneUtilMacros {
private class Macros(override val c: whitebox.Context) extends InlineMacros with HygieneUtilMacros {

This comment has been minimized.

Copy link
@oleg-py

oleg-py Jul 10, 2019

Collaborator

Is that okay to make that private? I don't even know how scoping works for macros.

This comment has been minimized.

Copy link
@Avasil

Avasil Jul 10, 2019

Author Collaborator

me neither but it shouldn't compile if it's wrong, right? Or is there a way it could still fail somewhere?

This comment has been minimized.

Copy link
@oleg-py

oleg-py Jul 10, 2019

Collaborator

That's the point, I have no clue 😆

This comment has been minimized.

Copy link
@Avasil

Avasil Jul 18, 2019

Author Collaborator

@oleg-py

I changed package of LocalSuite to something completely else and it passed
Also moved it to monix-eval subproject with different package (without monix prefix) and worked as well

Looks like it can be private as long as it is in scope of public methods accessing it. I asked someone with some experience with macros and that was his thought too

@oleg-py
Copy link
Collaborator

left a comment

Well, if macros work, I have no objections 😅

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

Cool, I'm merging it then but will leave issue open for now in case @alexandru wants to do more

@Avasil Avasil merged commit a579124 into monix:master Jul 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Avasil Avasil deleted the Avasil:hide-local-internals branch Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.