Skip to content

Conversation

Artem-Semenov-dev
Copy link

@Artem-Semenov-dev Artem-Semenov-dev commented May 27, 2024

The EnvSetting class is a tool that allows configuring the value that may differ across environment types. Previously it was an internal tool, but now it is available for all Spine projects.
Usage example:

    EnvSetting<UserReader> setting = new EnvSetting<>();
    setting.use(projectsUserReader(), Production.class)
           .use(localUserReader(), Local.class)
           .use(testsUserReader(), Tests.class);
    
    // ...

    // The instance is returned according to the current `Environment` type.
    final UserReader reader = setting.value(); 

This changeset includes the following changes in the EnvSetting:

  • introduced method chaining for a more fluent and convenient API (as shown in the example);
  • added a new method that allows retrieval of the stored value for the current environment type of the application;
  • made the API thread-safe, allowing concurrent read operations while ensuring exclusive access for write operations to maintain data integrity.

@Artem-Semenov-dev Artem-Semenov-dev requested a review from armiol May 27, 2024 15:25
@Artem-Semenov-dev Artem-Semenov-dev self-assigned this May 27, 2024
Copy link
Contributor

@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.

@Artem-Semenov-dev Mostly LGTM. Please see my comments.

readWriteExecutors.shutdownNow();
readWriteExecutors.awaitTermination(500, MILLISECONDS);
} catch (InterruptedException e) {
throw illegalStateWithCauseOf(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we want that, I would just ignore.

If you still think we need to re-throw, please have a method-level comment explaining the behaviour.

* the type of the environment
*/
void use(V value, Class<? extends EnvironmentType> type) {
public void use(V value, Class<? extends EnvironmentType> type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could return this? For call chaining, that is.

I am asking since the example from the PR description is a bit mouthful. I think I would look better like this:

    EnvSetting<UserReader> setting = new EnvSetting<>();
    setting.use(projectsUserReader(), Production.class)
           .use(localUserReader(), Local.class)
           .use(testsUserReader(), Tests.class);

@Artem-Semenov-dev Artem-Semenov-dev requested a review from armiol May 28, 2024 14:15
@armiol
Copy link
Contributor

armiol commented May 28, 2024

@Artem-Semenov-dev

   UserReader currentUserReader = setting.use(projectsUserReader(), Production.class)
                                          .use(localUserReader(), Local.class)
                                          .use(testsUserReader(), Tests.class)
                                          .value();

This should be an illustration of some real-world usage. In which (on Earth) scenario, one would call .value() right after .use()?

Copy link
Contributor

@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.

@Artem-Semenov-dev LGTM. However, please change the PR description according to my comment.

Copy link
Contributor

@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.

@Artem-Semenov-dev please update the README.md. There is a version number that you've changed.

@Artem-Semenov-dev Artem-Semenov-dev requested a review from armiol May 28, 2024 15:31
@Artem-Semenov-dev Artem-Semenov-dev merged commit ae0c60d into 1.x-dev May 28, 2024
@Artem-Semenov-dev Artem-Semenov-dev deleted the expose-env-setting branch May 28, 2024 15:49
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