Skip to content

Custom environment types#539

Merged
serhii-lekariev merged 38 commits intomasterfrom
env-types
Jun 4, 2020
Merged

Custom environment types#539
serhii-lekariev merged 38 commits intomasterfrom
env-types

Conversation

@serhii-lekariev
Copy link
Contributor

@serhii-lekariev serhii-lekariev commented May 26, 2020

This PR extends the Environment API to allow the definition of custom environment types. It also migrates the current boolean test/not-a-test setup to a class-based solution.

To register a new environment type, define a custom one like so:

import io.spine.base.EnvironmentType;

public final class Local extends EnvironmentType {
        
         // ...

        @Override
         protected boolean enabled() {
            // Check an environment variable or a file.
         }

        public static Local type() {
        // Obtain the instance somehow. Encouraged to be a singleton instance.
        }
  }
Environment environment = Environment.instance();

// As the type is not registered yet.
assertThat(environment.is(Local.type)).isFalse();

Environment.register(Local.type());

// Assuming that "Local" is actually enabled
assertThat(environment.is(Local.type)).isTrue();
assertThat(environment.type()).isSameInstanceAs(Local.type());

When checking the environment types, user-defined types are checked first. If no type was found to be enabled, falls back to io.spine.base.Production.

Deprecation notes

The following methods are now deprecated in favor of the new API:

  • isTests: use is(Tests.type());;
  • setToTests: use setTo(Tests.type());;
  • isProduction: use is(Production.type());;
  • setToProduction: use setTo(Production.type());.

@serhii-lekariev serhii-lekariev self-assigned this May 26, 2020
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #539 into master will increase coverage by 0.09%.
The diff coverage is 91.89%.

@@             Coverage Diff              @@
##             master     #539      +/-   ##
============================================
+ Coverage     73.71%   73.80%   +0.09%     
- Complexity     2936     2954      +18     
============================================
  Files           503      506       +3     
  Lines         11904    11954      +50     
  Branches        670      669       -1     
============================================
+ Hits           8775     8823      +48     
+ Misses         2905     2904       -1     
- Partials        224      227       +3     

@serhii-lekariev serhii-lekariev marked this pull request as ready for review May 26, 2020 10:32
@serhii-lekariev serhii-lekariev requested a review from armiol May 26, 2020 10:32
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev please see my comments.

@serhii-lekariev serhii-lekariev requested a review from armiol May 27, 2020 14:55
@serhii-lekariev
Copy link
Contributor Author

serhii-lekariev commented May 27, 2020

@armiol, PTAL again.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

Please see my comments.

/**
* Provides information about the environment (current platform used, etc.).
*
* <p><b>When extending, please note</b> that this class does not handle the situations when two
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rephrase in terms of the responsibitliy of the framework user. It's not that this class handles or does not handle any use cases. It just delegates the responsibility to tell between different PRODUCTION environments to the developer writing the code.

@serhii-lekariev serhii-lekariev requested a review from armiol May 29, 2020 10:13
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL again.

Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev please see my comments.

@@ -159,15 +159,15 @@ void mutateKnownEnvTypesOnce() {
@Test
@DisplayName("throw if a user attempts to create register the same environment twice")
void throwOnDoubleCreation() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a creation?

* {@link EnvironmentType#enabled()}. As such, if two or more user-defined environment types
* think that they are currently enabled, <b>the behaviour of {@link #currentType()} is
* undefined.</b>
* <p><b>When extending, please ensure</b> the mutual exclusivity on your {@code EnvironmentType}s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class cannot be extended. Please put a bit more into this thought.

@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL again.

@serhii-lekariev serhii-lekariev requested a review from armiol May 30, 2020 17:11
serhii-lekariev and others added 3 commits May 31, 2020 17:23
Shorten the `currentType()` to `type()` and un-mark the `register(type)` as `@Internal`.
…rentType()` to make it simpler for the callers.

Make `EnvironmentType` into an `abstract class` to ensure a more strict
access level of the `active` method.
Copy link
Collaborator

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev mostly LGTM. Please see my comments.

/**
* Returns the singleton instance.
*/
public static Production instance() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call it type().

Don't forget to update the code samples in the documentation.

|| stacktrace.contains("org.testng")) {
this.tests = true;
return true;
public boolean is(EnvironmentType type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's bring back isTests() and isProduction() and mark them @Deprecated in favor of ... .enabled().
It's a proper way of propagating the public API changes.

/**
* Returns the singleton instance of this class.
*/
public static Tests instance() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL again.

@serhii-lekariev serhii-lekariev requested a review from armiol June 4, 2020 12:24
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.

2 participants