-
Notifications
You must be signed in to change notification settings - Fork 642
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
Add possibility of configuration to Environment Listeners #843
Add possibility of configuration to Environment Listeners #843
Conversation
I think that I might be missing some tests here, but I'm not sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As usual from your PRs it's great, but I think we should allow all three options now.
kotlintest-extensions/src/main/kotlin/io/kotlintest/extensions/system/MapOverrides.kt
Outdated
Show resolved
Hide resolved
Agreed.
…On Fri, 28 Jun 2019, 12:05 Leonardo Colman Lopes, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
kotlintest-extensions/src/main/kotlin/io/kotlintest/extensions/system/MapOverrides.kt
<#843 (comment)>:
> @@ -1,12 +1,34 @@
package io.kotlintest.extensions.system
+
+enum class SystemOverrideMode { ALLOW_OVERRIDE, DENY_OVERRIDE }
My only point to this is that SET_OR_OVERRIDE should be set as default.
It's the default now, and I don't think we need to change that behavior and
potentially break clients
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#843>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFVSGR7JJIJ6MHBLFKKMK3P4ZAELANCNFSM4HZNP5UA>
.
|
@sksamuel I think it's done. Please review again! |
8bc00a8
to
c347945
Compare
I decided that setting the default to |
Do you want me to finish this off?
…On Wed, 10 Jul 2019 at 10:15, Leonardo Colman Lopes < ***@***.***> wrote:
I decided that setting the default to SetOrError is better. It should be
the expected, and if clients' code break, it is a good thing
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#843>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFVSGVN7AFENTS6Y7LYCBLP6X4KRANCNFSM4HZNP5UA>
.
|
Did I miss anything? |
Don't think so
…On Wed, 10 Jul 2019, 14:48 Leonardo Colman Lopes, ***@***.***> wrote:
Did I miss anything?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#843>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFVSGQZ4QSLUOEZGSZVZ63P6Y4HPANCNFSM4HZNP5UA>
.
|
Closes #829