8000 fix moon check --watch no update mbt.md by Young-Flash · Pull Request #821 · moonbitlang/moon · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix moon check --watch no update mbt.md #821

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 21, 2025
Merged

fix moon check --watch no update mbt.md #821

merged 1 commit into from
May 21, 2025

Conversation

Young-Flash
Copy link
Collaborator

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Potential race condition in watch mode handling

Category
Correctness
Code Snippet
moonbuild/src/watch.rs:146-150
if event.paths.iter().any(|p| {
p.ends_with(MOON_MOD_JSON)
|| p.ends_with(MOON_PKG_JSON)
|| p.display().to_string().ends_with(DOT_MBT_DOT_MD)
})
Recommendation
Consider using Path::extension() or a more robust path comparison:

p.extension().map_or(false, |ext| ext == "md") && p.to_string_lossy().ends_with(".mbt.md")

Reasoning
Using string-based path comparisons with endsWith can be fragile and may not handle all path formats correctly. Using proper path manipulation methods would be more robust.

Doc test patch generation logic spreading across multiple conditionals

Category
Maintainability
Code Snippet
moonutil/src/scan.rs:544-546
if !cur_pkg.is_third_party && !cur_pkg.mbt_md_files.is_empty() {
let pj_path = crate::doc_test::gen_md_test_patch(&cur_pkg, moonc_opt)?;
cur_pkg.doc_test_patch_file = pj_path;
}
Recommendation
Consider consolidating doc test patch logic into a dedicated method on Package:

impl Package {
    fn generate_doc_test_patches(&mut self, moonc_opt: &MooncOpt) -> Result<()> {
        if !self.is_third_party && !self.mbt_md_files.is_empty() {
            self.doc_test_patch_file = doc_test::gen_md_test_patch(self, moonc_opt)?;
        }
        Ok(())
    }
}

Reasoning
Moving this logic into a dedicated method would improve code organization and make it easier to maintain all doc test related functionality in one place.

Multiple string operations in file path checking

Category
Performance
Code Snippet
moonbuild/src/watch.rs:146-150
p.display().to_string().ends_with(DOT_MBT_DOT_MD)
Recommendation
Cache the string representation if needed multiple times:

let path_str = p.display().to_string();
if path_str.ends_with(MOON_MOD_JSON)
    || path_str.ends_with(MOON_PKG_JSON)
    || path_str.ends_with(DOT_MBT_DOT_MD)

Reasoning
Converting the path to a string multiple times is inefficient. If the string representation is needed multiple times, it should be cached in a variable.

@Young-Flash Young-Flash merged commit 9ab5989 into main May 21, 2025
13 checks passed
@Young-Flash Young-Flash deleted the fix_moon_check_watch_no_update_mbt_md branch May 21, 2025 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Young-Flash
0