internal: use sync_all #831

Young-Flash · 2025-05-28T03:50:30Z

No description provided.

peter-jerry-ye-code-review · 2025-05-28T03:50:41Z

Replacing flush() with sync_all() for more reliable file operations

Category
Correctness
Code Snippet
file.flush()
.context(format!("failed to flush {}", pj_path.display()))?;

// Changed to:
file.sync_all()?;
Recommendation
The change to sync_all() is good but should maintain the contextual error message:

file.sync_all()
.context(format!("failed to sync {}", pj_path.display()))?;
Reasoning
sync_all() is more robust than flush() as it ensures all data is written to disk and metadata is updated. However, dropping the context message makes debugging harder if errors occur.

Duplicate code in gen_doc_test_patch and gen_md_test_patch functions

Category
Maintainability
Code Snippet
Both functions contain identical file writing logic:
file.write_all(content.as_bytes())
.context(format!("failed to write to {}", pj_path.display()))?
file.sync_all()?;
Recommendation
Extract the common file writing logic into a separate function:

fn write_project_json(file: &mut File, content: String, path: &Path) -> anyhow::Result<()> {
file.write_all(content.as_bytes())
.context(format!("failed to write to {}", path.display()))?
file.sync_all()
.context(format!("failed to sync {}", path.display()))
}
Reasoning
Reducing code duplication improves maintainability and ensures consistent behavior across both test generation functions.

sync_all() may impact performance in test scenarios

Category
Performance
Code Snippet
file.sync_all()?;
Recommendation
Consider adding a configuration option to control sync behavior in test environments:

if config.force_sync {
file.sync_all().context(...)?
} else {
file.flush().context(...)?
}
Reasoning
sync_all() forces immediate disk writes which may not be necessary in all test scenarios and could slow down test execution. A configuration option would allow users to choose between safety and performance.

Young-Flash · 2025-05-28T05:44:06Z

crates/moonutil/src/doc_test.rs

@@ -316,8 +315,7 @@ pub fn gen_md_test_patch(pkg: &Package, moonc_opt: &MooncOpt) -> anyhow::Result<
    let content = serde_json_lenient::to_string_pretty(&pj)?;
    file.write_all(content.as_bytes())
        .context(format!("failed to write to {}", pj_path.display()))?;
-    file.flush()
-        .context(format!("failed to flush {}", pj_path.display()))?;
+    file.sync_all()?;


seems still don't work 🤔

https://github.com/moonbitlang/moon/actions/runs/15291063482/job/43010493886?pr=831#step:21:119

any ideas? cc @lynzrand @Guest0x0

Young-Flash enabled auto-merge (squash) May 28, 2025 03:50

Young-Flash commented May 28, 2025

View reviewed changes

Young-Flash force-pushed the use_sync_all branch 2 times, most recently from 63121a3 to 807d848 Compare May 29, 2025 06:18

internal: use sync_all

bfc9f11

Young-Flash force-pushed the use_sync_all branch from 2d8535d to bfc9f11 Compare May 30, 2025 02:21

Young-Flash closed this May 30, 2025

auto-merge was automatically disabled May 30, 2025 10:18
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

internal: use sync_all #831

internal: use sync_all #831

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

internal: use sync_all #831

internal: use sync_all #831

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!