Skip to content

Use resource bundle for reports #445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use resource bundle for reports #445

wants to merge 8 commits into from

Conversation

Lisrte
Copy link
Collaborator

@Lisrte Lisrte commented Mar 31, 2025

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A PR or issue has been opened in all impacted repositories (if any)

Does this PR already have an issue describing the problem?

#440

What kind of change does this PR introduce?

Refactor

What is the current behavior?

Use deprecated withMessageTemplate method.

What is the new behavior (if this is a feature change)?
Use resource bundle for reports

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

Lisrte added 4 commits March 31, 2025 10:59
Rename 'automaton' reports with 'automationSystem'

Signed-off-by: lisrte <[email protected]>
Signed-off-by: lisrte <[email protected]>
Fix commons pom.xml

Signed-off-by: lisrte <[email protected]>
@Lisrte Lisrte self-assigned this Mar 31, 2025
@Lisrte Lisrte requested a review from flo-dup March 31, 2025 15:33
@Lisrte Lisrte marked this pull request as ready for review March 31, 2025 15:33
Add TypedValue ID to static id reports

Signed-off-by: lisrte <[email protected]>
@@ -24,7 +24,7 @@ private BuilderReports() {

public static ReportNode createModelInstantiationReportNode(ReportNode reportNode) {
return reportNode.newReportNode()
.withMessageTemplate("modelInstantiation", "Model ${modelName} ${dynamicId} instantiation ${state}")
.withMessageTemplate("dynawo.dynasim.modelInstantiation")

Choose a reason for hiding this comment

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

For this report shouldn't we have 2 keys? One for the success case and one for the failure one?
There might still be a problem as we need to know during the instanciation of the ReportNode in which case we are one (maybe in the future we could change the API to allow to modify a ReportNode message template afterwards @flo-dup ).
But as it is right now the untyped value state of this report node will not be translated : successful or failed will appear even with french locale for instance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could admit a KO/OK string for now in all languages (rather than failed/successful). But that's one argument towards a message template key added after creation.

Lisrte added 2 commits April 7, 2025 14:53
Remove withResourceBundles call in non-root report node

Signed-off-by: lisrte <[email protected]>
@Lisrte Lisrte requested a review from flo-dup April 7, 2025 14:23
Copy link
Collaborator

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

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

There are still some en_US.properties files which could be removed

@Lisrte
Copy link
Collaborator Author

Lisrte commented Apr 11, 2025

There are still some en_US.properties files which could be removed

Fixed it.

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.

3 participants