Skip to content
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

refactor: Extract messages to the properties bundle #44

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jdneo
Copy link
Member

@jdneo jdneo commented Jul 18, 2023

No description provided.

@@ -61,8 +62,7 @@ public SourcesResult getBuildTargetSources(SourcesParams params) {
for (BuildTargetIdentifier btId : params.getTargets()) {
GradleBuildTarget target = buildTargetManager.getGradleBuildTarget(btId);
if (target == null) {
logger.warn("Skip sources collection for the build target: {}"
+ "because it cannot be found in the cache.", btId.getUri());
logger.warn(MessageUtils.get("warning.skipBuildTargetSources"), btId.getUri());

Choose a reason for hiding this comment

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

it's log message, not UI message. I don't think nls is needed for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends. For example, in the future, we can have a log appender that append the log to the client via BSP notification like OnBuildLogMessage, where users can see it.

On the other hand, if we are sure that no need to translate those log messages. We can simply ignore them in other language bundle files. The JVM will be fallback to the default if it does not exist.

Signed-off-by: Sheng Chen <[email protected]>
@jdneo jdneo marked this pull request as draft July 20, 2023 05:01
@@ -0,0 +1,7 @@
error.serverStorageMissing=The property 'buildServerStorage' is not set.
Copy link
Member

Choose a reason for hiding this comment

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

Good start for localization.

Copy link
Member

@akaroml akaroml left a comment

Choose a reason for hiding this comment

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

LGTM

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