Skip to content

Commit

Permalink
bug(fmt): do not iteratively call forc-fmt over members on the root d…
Browse files Browse the repository at this point in the history 
…ir (FuelLabs#4038)

## Description

closes FuelLabs#4027

This is a fix for the formatter being called to format multiple times.

This implements the suggested changes described in the issue, and
refactors some of the code:

- `format_file()` is now renamed to `write_file_formatted()`, since all
it does is write to a file.
- Extract logic of formatting a single file into `format_file()`. This
means its reusable at 3 levels: formatting a single file, formatting a
package and formatting a workspace.
- Handles formatting by workspace vs package - formatting a workspace is
handled slightly differently. Formatting a package has no changes - it
still works the same. However, for workspaces, we want to format all
files at the root, then find for each subdirectories (inclusively), all
the nested directories with a manifest inside. If it exists, we are
interested in formatting it. If the subdirectory happens to have a
`swayfmt.toml` inside it, we will prioritize that configuration file.
Generally, we should prefer member config > workspace config > default.
This nuance is [explained in a
comment](https://github.com/FuelLabs/sway/pull/4038/files#diff-bf9b5d7023fff817f83ae31bd01e66d2788178a0705a2c638ce79d7990cc374aR190-R193).
- Extract logic of formatting a manifest into `format_manifest()`

It's faster by quite a bit, with some good ol' primitive testing:


![image](https://user-images.githubusercontent.com/25565268/218050071-b6b5eb7f-9d82-4293-9321-996376a46a82.png)

new (fmt.sh):
```
for i in `seq 1 13`; do
  ./forc-fmt --path AMM
done
```

The above `forc-fmt` is built on `--release` and I moved it to the sway
apps repo for testing. Looped it 13 times because that's the no. of
projects in sway-applications.

old (fmt-old.sh):
```
for i in `seq 1 13`; do
  forc fmt --path AMM
done
```

This one is just the current formatter.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Kaya Gökalp <[email protected]>
  • Loading branch information
@eightfilms @sdankel @kayagokalp
3 people authored Feb 16, 2023
1 parent 1f9debf commit 080543e
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 69 deletions.
253 changes: 185 additions & 68 deletions forc-plugins/forc-fmt/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
use anyhow::{bail, Result};
use clap::Parser;
use forc_pkg::{manifest::ManifestFile, WorkspaceManifestFile};
use prettydiff::{basic::DiffOp, diff_lines};
use std::{
default::Default,
Expand Down Expand Up @@ -50,10 +51,22 @@ fn main() {
fn run() -> Result<()> {
let app = App::parse();

let dir = match app.path.as_ref() {
Some(path) => PathBuf::from(path),
None => std::env::current_dir()?,
};

let mut formatter = Formatter::from_dir(&dir)?;
if let Some(f) = app.file.as_ref() {
let file_path = &PathBuf::from(f);

// If we're formatting a single file, find the nearest manifest if within a project.
// Otherwise, we simply provide 'None' to format_file().
let manifest_file =
find_manifest_dir(file_path).map(|path| path.join(constants::MANIFEST_FILE_NAME));

if is_sway_file(file_path) {
format_single_file(file_path)?;
format_file(&app, file_path.to_path_buf(), manifest_file, &mut formatter)?;
return Ok(());
}

Expand All @@ -63,37 +76,182 @@ fn run() -> Result<()> {
);
};

let dir = match app.path.as_ref() {
Some(path) => PathBuf::from(path),
None => std::env::current_dir()?,
};

let manifest_file = forc_pkg::manifest::ManifestFile::from_dir(&dir)?;
for (_, member_path) in manifest_file.member_manifests()? {
let member_dir = member_path.dir();
let mut formatter = Formatter::from_dir(member_dir)?;
format_pkg_at_dir(&app, &dir, &mut formatter)?;

match manifest_file {
ManifestFile::Workspace(ws) => {
format_workspace_at_dir(&app, &ws, &dir)?;
}
ManifestFile::Package(_) => {
format_pkg_at_dir(&app, &dir, &mut formatter)?;
}
}

Ok(())
}

fn format_single_file(path: &Path) -> Result<()> {
let mut formatter = Formatter::default();
if let Ok(file_content) = fs::read_to_string(path) {
match Formatter::format(&mut formatter, file_content.into(), None) {
/// Recursively get a Vec<PathBuf> of subdirectories that contains a Forc.toml.
fn get_sway_dirs(workspace_dir: PathBuf) -> Vec<PathBuf> {
let mut dirs_to_format = vec![];
let mut dirs_to_search = vec![workspace_dir];

while let Some(next_dir) = dirs_to_search.pop() {
if let Ok(read_dir) = fs::read_dir(next_dir) {
for entry in read_dir.filter_map(|res| res.ok()) {
let path = entry.path();

if path.is_dir() {
dirs_to_search.push(path.clone());
if path.join(constants::MANIFEST_FILE_NAME).exists() {
dirs_to_format.push(path);
}
}
}
}
}

dirs_to_format
}

/// Format a file, given its path.
/// Returns:
/// - Ok(true) if executed successfully and formatted,
/// - Ok(false) if executed successfully and not formatted,
/// - Err if it fails to execute at all.
fn format_file(
app: &App,
file: PathBuf,
manifest_file: Option<PathBuf>,
formatter: &mut Formatter,
) -> Result<bool> {
let file = file.canonicalize()?;
if let Ok(file_content) = fs::read_to_string(&file) {
let mut edited = false;
let file_content: Arc<str> = Arc::from(file_content);
let build_config = manifest_file.map(|f| {
BuildConfig::root_from_file_name_and_manifest_path(
file.clone(),
f,
BuildTarget::default(),
)
});
match Formatter::format(formatter, file_content.clone(), build_config.as_ref()) {
Ok(formatted_content) => {
format_file(path, &formatted_content)?;
if app.check {
if *file_content != formatted_content {
info!("File was edited by formatter: \n{:?}\n", file);
display_file_diff(&file_content, &formatted_content)?;
edited = true;
}
} else {
write_file_formatted(&file, &formatted_content)?;
}

return Ok(edited);
}
Err(err) => {
error!("{}\n", err);
// there could still be Sway files that are not part of the build
error!(
"\nThis file: {:?} is not part of the build\n{}\n",
file, err
);
}
}
}

bail!("Could not read file: {:?}", file)
}

/// Format the workspace at the given directory.
fn format_workspace_at_dir(app: &App, workspace: &WorkspaceManifestFile, dir: &Path) -> Result<()> {
let mut contains_edits = false;
let mut formatter = Formatter::from_dir(dir)?;
let mut members = vec![];

for member_path in workspace.member_paths()? {
members.push(member_path)
}

// Format files at the root - we do not want to start calling format_pkg_at_dir() here,
// since this would mean we format twice on each subdirectory.
if let Ok(read_dir) = fs::read_dir(dir) {
for entry in read_dir.filter_map(|res| res.ok()) {
let path = entry.path();
if is_sway_file(&path) {
format_file(
app,
path,
Some(workspace.dir().to_path_buf()),
&mut formatter,
)?;
}
}
}

// Format subdirectories. We do not call format on members directly here, since
// in workspaces, it is perfectly valid to have subdirectories containing Sway files,
// yet not be a member of the workspace.
for sub_dir in get_sway_dirs(dir.to_path_buf()) {
if sub_dir.join(constants::MANIFEST_FILE_NAME).exists() {
// Here, we cannot simply call Formatter::from_dir() and rely on defaults
// if there is no swayfmt.toml in the sub directory because we still want
// to use the swayfmt.toml at the workspace root (if any).
// In order of priority: member > workspace > default.
formatter = Formatter::from_dir(&sub_dir)?;
}
format_pkg_at_dir(app, &sub_dir, &mut formatter)?;
}

let manifest_file = dir.join(constants::MANIFEST_FILE_NAME);

// Finally, format the root manifest using taplo formatter
if let Ok(edited) = format_manifest(app, manifest_file) {
contains_edits = edited;
}

if app.check && contains_edits {
// One or more files are not formatted, exit with error
bail!("Files contain formatting violations.");
}

Ok(())
}

/// Format the given manifest at a path.
/// Returns:
/// - Ok(true) if executed successfully and formatted,
/// - Ok(false) if executed successfully and not formatted,
/// - Err if it fails to execute at all.
fn format_manifest(app: &App, manifest_file: PathBuf) -> Result<bool> {
if let Ok(manifest_content) = fs::read_to_string(&manifest_file) {
let mut edited = false;
let taplo_alphabetize = taplo_fmt::Options {
reorder_keys: true,
..Default::default()
};
let formatted_content = taplo_fmt::format(&manifest_content, taplo_alphabetize);
if !app.check {
write_file_formatted(&manifest_file, &formatted_content)?;
} else if formatted_content != manifest_content {
edited = true;
error!(
"Improperly formatted manifest file: {}",
manifest_file.display()
);
display_file_diff(&manifest_content, &formatted_content)?;
} else {
info!(
"Manifest Forc.toml formatted correctly: {}",
manifest_file.display()
)
}

return Ok(edited);
};

bail!("failed to format manifest")
}

/// Format the package at the given directory.
fn format_pkg_at_dir(app: &App, dir: &Path, formatter: &mut Formatter) -> Result<()> {
match find_manifest_dir(dir) {
Expand All @@ -104,62 +262,21 @@ fn format_pkg_at_dir(app: &App, dir: &Path, formatter: &mut Formatter) -> Result
let mut contains_edits = false;

for file in files {
if let Ok(file_content) = fs::read_to_string(&file) {
let file_content: Arc<str> = Arc::from(file_content);
let build_config = BuildConfig::root_from_file_name_and_manifest_path(
file.clone(),
manifest_path.clone(),
BuildTarget::default(),
);
match Formatter::format(formatter, file_content.clone(), Some(&build_config)) {
Ok(formatted_content) => {
if app.check {
if *file_content != formatted_content {
contains_edits = true;
info!("\n{:?}\n", file);
display_file_diff(&file_content, &formatted_content)?;
}
} else {
format_file(&file, &formatted_content)?;
}
}
Err(err) => {
// there could still be Sway files that are not part of the build
error!("\nThis file: {:?} is not part of the build", file);
error!("{}\n", err);
}
}
}
if let Ok(edited) = format_file(app, file, Some(manifest_file.clone()), formatter) {
contains_edits = edited;
};
}
// format manifest using taplo formatter
if let Ok(file_content) = fs::read_to_string(&manifest_file) {
let taplo_alphabetize = taplo_fmt::Options {
reorder_keys: true,
..Default::default()
};
let formatted_content = taplo_fmt::format(&file_content, taplo_alphabetize);
if !app.check {
format_file(&manifest_file, &formatted_content)?;
} else if formatted_content != file_content {
contains_edits = true;
error!("\nManifest Forc.toml improperly formatted");
display_file_diff(&file_content, &formatted_content)?;
} else {
info!("\nManifest Forc.toml properly formatted")
}
if let Ok(edited) = format_manifest(app, manifest_file) {
contains_edits = contains_edits || edited;
}

if app.check {
if contains_edits {
// One or more files are not formatted, exit with error
bail!("Files contain formatting violations.");
} else {
// All files are formatted, exit cleanly
Ok(())
}
} else {
Ok(())
if app.check && contains_edits {
// One or more files are not formatted, exit with error
bail!("Files contain formatting violations.");
}

Ok(())
}
_ => bail!("Manifest file does not exist"),
}
Expand Down Expand Up @@ -205,7 +322,7 @@ fn display_file_diff(file_content: &str, formatted_content: &str) -> Result<()>
Ok(())
}

fn format_file(file: &Path, formatted_content: &str) -> Result<()> {
fn write_file_formatted(file: &Path, formatted_content: &str) -> Result<()> {
fs::write(file, formatted_content)?;

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion forc-util/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ where

pub fn is_sway_file(file: &Path) -> bool {
let res = file.extension();
Some(OsStr::new(constants::SWAY_EXTENSION)) == res
file.is_file() && Some(OsStr::new(constants::SWAY_EXTENSION)) == res
}

pub fn find_file_name<'sc>(manifest_dir: &Path, entry_path: &'sc Path) -> Result<&'sc Path> {
Expand Down

0 comments on commit 080543e

Please sign in to comment.