refactor: cleanup zhconv and linguist file. #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @yyc12345. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @yyc12345 - I've reviewed your changes - here's some feedback:
- Consider splitting similarly large refactorings into multiple, smaller PRs in the future for easier review.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let source_msg_count = source_catalog.count(); | ||
| if target_msg_count != source_msg_count { | ||
| return Err(CmdError::DifferentMessages(language_code, source_msg_count, target_msg_count)); | ||
| }; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the common translation looping logic into a helper trait implemented by both TS and PO types to reduce code duplication.
The added dispatch in ZhConvFile and the near-duplicate translation loops for TS and PO files increase the complexity. To simplify and reduce duplication, consider extracting the common looping logic into a helper (or trait) that both TS and PO types implement. For example, you could define a new trait for translating messages:
trait Translator {
fn messages_count(&self) -> usize;
fn iterate_messages_mut<'a>(&'a mut self) -> Box<dyn Iterator<Item = (&'a mut dyn MessageMut, &'a dyn Message)> + 'a>;
fn get_language(&self) -> String;
}
trait MessageMut {
fn is_translated(&self) -> bool;
fn is_plural(&self) -> bool;
fn set_translation(&mut self, msg: String) -> Result<(), CmdError>;
}
fn translate_generic<T: Translator>(source: &T, target: &mut T) -> Result<(), CmdError> {
let language = target.get_language();
if source.messages_count() != target.messages_count() {
return Err(CmdError::DifferentMessages(language, source.messages_count(), target.messages_count()));
}
for (target_msg, source_msg) in target.iterate_messages_mut().zip(source.iterate_messages_mut()) {
if target_msg.0.is_translated() {
continue;
}
if !target_msg.0.is_plural() && source_msg.1.is_translated() {
// Assuming source_msg.1 provides a method to get the translation string.
let src_str = /* extract string from source_msg.1 */;
let translated = zhconv_wrapper(&src_str, &language)?;
target_msg.0.set_translation(translated)?;
}
}
Ok(())
}Then update your translate_content_based_on implementations for both TS and PO to delegate to this helper. This refactoring centralizes the common behavior, reducing the number of places that need to be maintained while keeping functionality intact.
Reviewer's GuideThis pull request refactors the handling of translation files (Qt Linguist .ts and Gettext .po) by introducing a new File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
yyc12345
force-pushed
the
patch
branch
2 times, most recently
from
0a476e1 to
a72a917
Compare
- use `&str` and `&Path` instead of `&String` and `&PathBuf` in files changed this time.
- rename something to make they are more in line with Rust's expressive habits.
* rename `CmdStatsError` to `CmdError` in `subcmd::statistics`. it can be shorten without any name conflict.
* remove the `Error` tail of members in some error enum.
* correct `LoadPO` and `LoadPo`. Rust doesn't suggest left style.
* correct `FileLoad` to `LoadFile`. Rust suggest that the verb should be the first position.
- create a new module folder `i18n_file` for holding various translation file logic.
* create a `linguist` module for holding Qt Linguist TS file logic.
* create a `gettext` module for holding GNU Gettext PO file logic.
* create a `common` module for holding these 2 file types shared components.
- move `linguist_file`
* most content are moved into `i18n_file::linguist`.
* rename `TsMessageStats` to `MessageStats` and move it to `i18n_file::common`
* remove useless import, such as `std::u64`.
* create new function `get_language` for `Ts` to have uniform interface.
* move `correct_language_code` to `subcmd::zhconv` because only this module use it.
- move Gettext PO file logic.
* extract logic involving load, load with fallback, save, from `subcmd::zhconv`.
* extract logic involving generate statistics from `subcmd::statistics`.
* and put them into `i18n_file::gettext` with a proper wrapper (not in raw gettext catalog).
- cleanup `subcmd::zhconv` module
* remove fat `ZhConvertible`, use lighter `ZhConvFile` enum instead.
* clean all `expect()`
* move the logic about translating PO content into a function `translate_po_content` and optimize its performance.
- extract the function, that guess the translation file type from the file extension, from `subcmd::{zhconv, statistics}` and put into `i18n_file::common` with a new created enum `I18nFileKind`.
- move load, load with fallback, save logic into Ts and Po inside.
- add testing stuff.
* add a load from string feature for Ts and Po only in test mode for convenient test.
* add 2 unittest for new organised PO file logic in `subcmd::zhconv` and `i18n_file::gettext` respectively like TS file unittest.
- improve performances.
* optimize ts load function. use `std::io::Read` trait instead of reading the whole file before parsing it.
* optimize `subcmd::zhconv::translate_po_content` by using `Iterator::zip()` and caching length to reduce useless iterator calling.
- modify other modules according to hereinabove said changes to make project can be built.
BLumia
approved these changes
May 12, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, yyc12345 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
&strand&Pathinstead of&Stringand&PathBufin files changed this time.CmdStatsErrortoCmdErrorinsubcmd::statistics. it can be shorten without any name conflict.Errortail of members in some error enum.LoadPOandLoadPo. Rust doesn't suggest left style.FileLoadtoLoadFile. Rust suggest that the verb should be the first position.trfilefor holding various translation file logic.linguistmodule for holding Qt Linguist TS file logic.gettextmodule for holding GNU Gettext PO file logic.commonmodule for holding these 2 file types shared components.linguist_filetrfile::linguist.TsMessageStatstoMessageStatsand move it totrfile::commonstd::u64.get_languageforTsto have uniform interface.correct_language_codetosubcmd::zhconvbecause only this module use it.subcmd::zhconv.subcmd::statistics.trfile::gettextwith a proper wrapper (not in raw gettext catalog).subcmd::zhconvmoduleZhConvertible, use lighterZhConvFileenum instead.expect()translate_po_contentand optimize its performance.subcmd::{zhconv, statistics}and put intotrfile::commonwith a new created enumTrFileKind.subcmd::zhconvandtrfile::gettextrespectively like TS file unittest.std::io::Readtrait instead of reading the whole file before parsing it.subcmd::zhconv::translate_po_contentby usingIterator::zip()and caching length to reduce useless iterator calling.Summary by Sourcery
Refactor and reorganize translation file handling modules, improving code structure and maintainability by introducing a new
trfilemodule with separate components for different translation file types.New Features:
trfilemodule with separate submodules for different translation file typesTrFileKindenum for detecting translation file typesEnhancements:
Chores: