Skip to content

Register EnvironmentTypes using their classes#545

Merged
serhii-lekariev merged 45 commits intomasterfrom
stricter-env-api
Jun 13, 2020
Merged

Register EnvironmentTypes using their classes#545
serhii-lekariev merged 45 commits intomasterfrom
stricter-env-api

Conversation

@serhii-lekariev
Copy link
Contributor

@serhii-lekariev serhii-lekariev commented Jun 10, 2020

This PR introduces several changes to Environment and EnvironmentTypes.

  • Tests and Production now have package-private constructors;
  • Environment now caches the class of the active EnvironmentType instead of an instance;
  • Environment now exposes an @Internal method to register an EnvironmentType by class. Classes to be registered using this method must have a parameterless constructor. This limitation is caused by the fact that we have to instantiate the specified EnvironmentType.

This leads to a signature change: public EnvironmentType type() -> public Class<? extends EnvironmentType> type().
It also allows to add a setTo(Class<? extends EnvironmentType>) method, which is now necessary, as users can no longer setTo(new Tests()), as Tests now has a package-private constructor.

Invokables

This PR also extracts some of the reflection-related utilities into an Invokables class.

Users of Methods may found the utilities there.

@serhii-lekariev serhii-lekariev self-assigned this Jun 10, 2020
@serhii-lekariev serhii-lekariev requested a review from armiol June 10, 2020 14:42
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL.

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #545 into master will increase coverage by 0.03%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master     #545      +/-   ##
============================================
+ Coverage     73.79%   73.82%   +0.03%     
- Complexity     2951     2959       +8     
============================================
  Files           506      506              
  Lines         11943    11972      +29     
  Branches        669      669              
============================================
+ Hits           8813     8838      +25     
- Misses         2903     2907       +4     
  Partials        227      227              

return register(envTypeInstance);
} catch (NoSuchMethodException | IllegalAccessException |
InstantiationException | InvocationTargetException e) {
String message = "Could not register environment type `%s` by class. You may try " +
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 say something about the parameterless ctor in the message.

@serhii-lekariev serhii-lekariev requested a review from armiol June 11, 2020 08:42
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL again.

@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL again.

only check for it to instantiate it.

Clean up, add and tweak documentation.
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL.

* determine whether it's enabled} later.
*
* <p>The specified {@code type} must have a parameterless constructor.
* {@linkplain EnvironmentType#enabled() activity} of the specified environment type is going
Copy link
Collaborator

Choose a reason for hiding this comment

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

activity should start from the capital letter. But I am not sure I understand this sentence:

activity of the specified environment type is going to be checked against an instance created by invoking the parameterless constructor.

Please rewrite into two or more simple sentences.

* assertThat(environment.is(AwsLambda.class)).isFalse();
* </pre>
*
* <p>{@linkplain #setTo(EnvironmentType) explicitly setting} the environment type overrides
Copy link
Collaborator

Choose a reason for hiding this comment

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

When set {@linkplain ... explicitly}, ...

Notice the capital letter.

* <p>{@code Environment} caches the {@code EnvironmentType} once its calculated.
* This means that if one environment type has been found to be active, its instance is saved.
* If later it becomes logically inactive, e.g. the environment variable that's used to check the
* environment type changes, {@code Environment} is still going to return the cached value, unless
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use complex sentences.

... going to return the cached value. To overwrite the value (do something). Also, the value may be {@linkplain ... reset}.

@serhii-lekariev serhii-lekariev requested a review from armiol June 12, 2020 17:38
@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 LGTM with a single comment. Please make sure to bump time after this one is merged as well.

AtomicBoolean envCached = new AtomicBoolean(false);
assertThat(environment.is(Tests.class));
Thread thread = new Thread(() -> {
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please convert this to the method-level comment.

Don't use inline comments except for emergency cases.

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 to the related PR in core-java first.

@serhii-lekariev serhii-lekariev requested a review from armiol June 13, 2020 14:49
@serhii-lekariev
Copy link
Contributor Author

@armiol, PTAL.

/**
* Sets the current environment type to {@code type.getClass()}. Overrides the current value.
*
* <p>Calls {@link #register(EnvironmentType)} internally.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the supplied type was not {@linkplain #register(EnvironmentType) registered} previously, it is registered.

/**
* Sets the current environment type to the specified one. Overrides the current value.
*
* <p>Calls {@link #register(Class)} internally.
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 serhii-lekariev merged commit f48c5cd into master Jun 13, 2020
@serhii-lekariev serhii-lekariev deleted the stricter-env-api branch June 13, 2020 15:26
@dmitrykuzmin dmitrykuzmin mentioned this pull request Sep 8, 2020
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