Skip to content

Conversation

@1993heqiang
Copy link
Contributor

Polishing: Extract repeated code into a method to make the code more concise.

@markpollack
Copy link
Member

I would chain the constructors instead and not call out to a utility method.

@1993heqiang
Copy link
Contributor Author

1993heqiang commented Nov 13, 2024

I would chain the constructors instead and not call out to a utility method.

@markpollack I have modified it according to your suggestion. Please help me review it again.

@1993heqiang
Copy link
Contributor Author

@markpollack
I saw a test case is disabled, do you think this modification can solve the problem?

Test Case:

	@Disabled("Need to improve PromptTemplate to better handle Resource toString and tracking with 'dynamicModel' for underlying StringTemplate")
	@Test
	public void testRenderResourceAsValue() throws Exception {
		...
	}

Fix code:

	public PromptTemplate(String template, Map<String, Object> model) {
		Assert.notNull(template, "template must not be null");
		Assert.notNull(model, "model must not be null");
		this.template = template;
		// If the template string is not valid, an exception will be thrown
		try {
			STGroup stGroup = new STGroup('{', '}');
			// Register a custom renderer for Resource objects
			stGroup.registerRenderer(Resource.class, (value, formatString, locale) -> readContentFromResource(value));
			this.st = new ST(stGroup, this.template);
			for (Entry<String, Object> entry : model.entrySet()) {
				add(entry.getKey(), entry.getValue());
			}
		}
		catch (Exception ex) {
			throw new IllegalArgumentException("The template string is not valid.", ex);
		}
	}

	private static String readContentFromResource(Resource resource) {
		Assert.notNull(resource, "resource must not be null");
		try {
			return resource.getContentAsString(Charset.defaultCharset());
		}
		catch (IOException ex) {
			throw new RuntimeException("Failed to read resource", ex);
		}
	}

@1993heqiang
Copy link
Contributor Author

@markpollack @ilayaperumalg @sobychacko This PR has been open for a long time. If you think this is not an issue, I will close this PR.

@1993heqiang 1993heqiang closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants