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

feat: dprint formatter #3820

Merged
merged 9 commits into from
Jan 30, 2020
Merged

feat: dprint formatter #3820

merged 9 commits into from
Jan 30, 2020

Conversation

bartlomieju
Copy link
Member

Closes #3818

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Jan 29, 2020

Okay, the file matcher was designed like this:

  • Accept a list of globs in CLI. The default should be ["."].
  • Expand these globs.
    • For files that are matched, add these files to the list of matches.
    • For directories that are matched, expand some list of "default" globs rooted on each directory. This time only match files. Add these files to the list of matches.
      • For the formatter, the default globs are ["**/*"].
      • For the test runner, the default globs are ["**/?(*_)test.{js,ts}"].
      • Both have just one default glob, so unless it's trivial you can support one. But keep in mind that the patterns supported by just one glob pattern are quite limited.
  • Apply exclude globs, accepted as a CSV list. Excludes must support path prefixes -- any path which has a parent which matches an exclude glob must not be included.

See denoland/std#604 for the defence of this design.

@bartlomieju
Copy link
Member Author

Okay, the file matcher was designed like this:

  • Accept a CSV list of globs in CLI. The default should be ["."].

  • Expand these globs.

    • For files that are matched, add these files to the list of matches.

    • For directories that are matched, expand some list of "default" globs rooted on each directory. This time only match files. Add these files to the list of matches.

      • For the formatter, the default globs are ["**/*"].
      • For the test runner, the default globs are ["**/?(*_)test.{js,ts}"].
      • Both use just one glob pattern, so unless it's trivial you can support one default glob. But keep in mind that the patterns supported by just one glob pattern are quite limited.

See denoland/deno_std#604 for the defence of this design.

Thanks for detailed explanation @nayeemrmn - PTAL at the PR, I added basic support for list of globs and formatter uses exact glob you specified above (though it should be limited to .tsx? and .jsx? as dprint does not support yet markdown). IIRC both formatter and test runner support ignore/exclude lists - I think I'll skip them in this PR and open an issue for that for later.

@nayeemrmn
Copy link
Collaborator

Edited my above comment with exclude semantics.

cli/formatter.rs Outdated
Comment on lines 127 to 132
for path in glob_paths {
let files = glob::glob(&path)
.expect("Failed to execute glob.")
.filter_map(Result::ok);
target_files.extend(files);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like:

  for path in glob_paths {
    let matches = glob::glob(&path)
      .expect("Failed to execute glob.")
      .filter_map(Result::ok);
    for match in matches {
      if is_dir(&match) {
        let secondary_matches = glob::glob(&format("{}/**/*", &match))
          .expect("Failed to execute glob.")
          .filter_map(Result::ok);
        for secondary_match in secondary_matches {
          if !is_dir(&secondary_match) {
            target_files.push(secondary_match);
          }
        }
      } else {
        target_files.push(match);
      }
    }
  }

@bartlomieju bartlomieju requested a review from ry January 29, 2020 21:45
@bartlomieju bartlomieju changed the title [WIP] feat: dprint formatter feat: dprint formatter Jan 29, 2020
tools/fmt_test.py Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/formatter.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

@nayeemrmn @ry there are two flags that are not present in this PR that are pretty useful: --stdout and --ignore-path=<FILE...>. Let's do them in follow up PR.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 73a3cc2 into denoland:master Jan 30, 2020
@bartlomieju bartlomieju deleted the dprint_formatter branch January 30, 2020 08:29
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //std/prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
* rewrite fmt_test in Rust, remove tools/fmt_test.py
* remove //prettier
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.

replace prettier with dprint for "deno fmt"
4 participants