Skip to content

Add caching to Version class#124

Open
Valkryst wants to merge 7 commits intojinput:masterfrom
Valkryst:update-version-class
Open

Add caching to Version class#124
Valkryst wants to merge 7 commits intojinput:masterfrom
Valkryst:update-version-class

Conversation

@Valkryst
Copy link
Contributor

@Valkryst Valkryst commented Nov 1, 2023

The primary goal of this was to cache the result of Version.getVersion(), so that we do not reload the value from the JAR whenever getVersion() is called.

As we were already loading the entire pom.properties file, I created a class to load and expose all available values from that file. The Version class was then rewritten as a wrapper, for backwards compatibility, to use the new PomProperties class.

properties.load(is);
} catch (final IOException | NullPointerException e) {
final Logger logger = Logger.getLogger(PomProperties.class.getName());
logger.warning("Failed to load properties file: " + PROPERTIES_FILE_PATH);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should never occur, as far as I'm aware. However, I thought it better to warn developers about the failure rather than silently ignoring/handling it as the original implementation did.

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class PomPropertiesTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests could be more thorough, but we would need to do some ugly mocking of the getResourceAsStream function to pass-in an InputStream with test data. I can implement this if we want, but I'm open to other suggestions if there's a more straight forward way to pass in test data to the PomProperties.loadProperties() function.

* You acknowledge that this software is not designed or intended for use in the
* design, construction, operation or maintenance of any nuclear facility.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed okay to remove as the class was totally rewritten.

return INSTANCE.properties.getProperty("groupId");
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we using semantic versioning? I could omit most of this JavaDoc and instead point a link to some other page which explains things

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.

1 participant