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

[JEP-216] Extract localization support infrastructure into localization-support plugin #31

Merged
merged 23 commits into from
May 2, 2019

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Nov 11, 2018

Work in progress: localization-support is not yet hosted or released. Some commits need to be reverted before this is merged.

Downstream from kohsuke/localizer#16, jenkinsci/stapler#156, jenkinsci/jenkins#3729, and https://github.com/daniel-beck/localization-support-plugin

Also update the resource files with the current state from Jenkins core, as they're being removed in the PR linked above.

Later revisions of this PR also support webapp resources, both from plugins and from core. Examples:

Core webapp

Before

before-core

After

after-core

Plugin webapp

Before

before-plugin

After

(with placeholder resource)

after-plugin

@LinuxSuRen
Copy link
Member

LocalizationFacet and I18nRequestDispatcher exists for help resource files. But it's not the best solution.

@daniel-beck
Copy link
Member Author

@LinuxSuRen Got it. I just was confused because the help files were still in core, as well as in this plugin, so I thought this worked when it did not. There's quite a lot of duplication going on.

@LinuxSuRen
Copy link
Member

@daniel-beck Actually it's working partially. So I did not remove those files from the core.

@LinuxSuRen
Copy link
Member

@daniel-beck We should figure out a way to solve a similar situation. I found the exception earlier. He should send the message if someone made a change of localization key or parameters.

@daniel-beck
Copy link
Member Author

daniel-beck/localization-support-plugin@492bb16 looks like it handles the problem of most localized help files.

The only ones still being a problem seem to be webapp/ resources.

@daniel-beck daniel-beck changed the title Extract localization support infrastructure into localization-support plugin [JEP-216] Extract localization support infrastructure into localization-support plugin Feb 6, 2019
Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

LGTM
They're all good except those help files were come from Descriptor. Like:

http://localhost:8080/jenkins/descriptor/hudson.ProxyConfiguration/help/port

But I think we can merge this. Then fix that later.

@daniel-beck daniel-beck changed the title [JEP-216] Extract localization support infrastructure into localization-support plugin DO NOT MERGE: [JEP-216] Extract localization support infrastructure into localization-support plugin May 2, 2019
@daniel-beck
Copy link
Member Author

@LinuxSuRen Thanks for identifying this problem, you're right.

It seems I originally did that in jenkinsci/localization-support-plugin@492bb16 but removed it again in jenkinsci/localization-support-plugin@a5b4e4f#diff-ba7f3e4f89111bd652dffe39ab80d9c7 because I didn't remember why it was needed.

Will fix in localization-support and integrate it here.

@daniel-beck
Copy link
Member Author

@LinuxSuRen The PR linked in the latest commit should restore this functionality. Could you test the current state of this PR in depth? I expect all kinds localizations to be supported as it is, with the build of localization-support integrated in 8cf2438

@LinuxSuRen
Copy link
Member

@daniel-beck Of course. I'll test this by manul.

@daniel-beck daniel-beck changed the title DO NOT MERGE: [JEP-216] Extract localization support infrastructure into localization-support plugin [JEP-216] Extract localization support infrastructure into localization-support plugin May 2, 2019
Copy link
Member

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