Skip to content

Commit

Permalink
feat(publish): give diagnostic on invalid package files
Browse files Browse the repository at this point in the history
  • Loading branch information
lucacasonato committed Jan 24, 2024
1 parent 176118a commit e2ad80b
Show file tree
Hide file tree
Showing 16 changed files with 441 additions and 86 deletions.
46 changes: 25 additions & 21 deletions cli/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::borrow::Cow;
use std::fmt;
use std::fmt::Display;
use std::fmt::Write as _;
use std::path::PathBuf;

use deno_ast::ModuleSpecifier;
use deno_ast::SourcePos;
Expand Down Expand Up @@ -61,17 +62,19 @@ impl DiagnosticSourcePos {

#[derive(Clone, Debug)]
pub enum DiagnosticLocation<'a> {
/// The diagnostic is relevant to an entire file.
File {
/// The diagnostic is relevant to a specific path.
Path { path: PathBuf },
/// The diagnostic is relevant to an entire module.
Module {
/// The specifier of the module that contains the diagnostic.
specifier: Cow<'a, ModuleSpecifier>,
},
/// The diagnostic is relevant to a specific position in a file.
/// The diagnostic is relevant to a specific position in a module.
///
/// This variant will get the relevant `SouceTextInfo` from the cache using
/// the given specifier, and will then calculate the line and column numbers
/// from the given `SourcePos`.
PositionInFile {
ModulePosition {
/// The specifier of the module that contains the diagnostic.
specifier: Cow<'a, ModuleSpecifier>,
/// The source position of the diagnostic.
Expand All @@ -80,13 +83,6 @@ pub enum DiagnosticLocation<'a> {
}

impl<'a> DiagnosticLocation<'a> {
fn specifier(&self) -> &ModuleSpecifier {
match self {
DiagnosticLocation::File { specifier } => specifier,
DiagnosticLocation::PositionInFile { specifier, .. } => specifier,
}
}

/// Return the line and column number of the diagnostic.
///
/// The line number is 1-indexed.
Expand All @@ -97,8 +93,9 @@ impl<'a> DiagnosticLocation<'a> {
/// everyone uses VS Code. :)
fn position(&self, sources: &dyn SourceTextStore) -> Option<(usize, usize)> {
match self {
DiagnosticLocation::File { .. } => None,
DiagnosticLocation::PositionInFile {
DiagnosticLocation::Path { .. } => None,
DiagnosticLocation::Module { .. } => None,
DiagnosticLocation::ModulePosition {
specifier,
source_pos,
} => {
Expand Down Expand Up @@ -384,7 +381,7 @@ fn print_diagnostic(
write!(
io,
"{}",
colors::yellow(format_args!("warning[{}]", diagnostic.code()))
colors::yellow_bold(format_args!("warning[{}]", diagnostic.code()))
)?;
}
}
Expand All @@ -410,11 +407,18 @@ fn print_diagnostic(
RepeatingCharFmt(' ', max_line_number_digits as usize),
colors::intense_blue("-->"),
)?;
let location_specifier = location.specifier();
if let Ok(path) = location_specifier.to_file_path() {
write!(io, " {}", colors::cyan(path.display()))?;
} else {
write!(io, " {}", colors::cyan(location_specifier.as_str()))?;
match &location {
DiagnosticLocation::Path { path } => {
write!(io, " {}", colors::cyan(path.display()))?;
}
DiagnosticLocation::Module { specifier }
| DiagnosticLocation::ModulePosition { specifier, .. } => {
if let Ok(path) = specifier.to_file_path() {
write!(io, " {}", colors::cyan(path.display()))?;
} else {
write!(io, " {}", colors::cyan(specifier.as_str()))?;
}
}
}
if let Some((line, column)) = location.position(sources) {
write!(
Expand Down Expand Up @@ -614,7 +618,7 @@ mod tests {
specifier: specifier.clone(),
text_info,
};
let location = super::DiagnosticLocation::PositionInFile {
let location = super::DiagnosticLocation::ModulePosition {
specifier: Cow::Borrowed(&specifier),
source_pos: super::DiagnosticSourcePos::SourcePos(pos),
};
Expand All @@ -631,7 +635,7 @@ mod tests {
specifier: specifier.clone(),
text_info,
};
let location = super::DiagnosticLocation::PositionInFile {
let location = super::DiagnosticLocation::ModulePosition {
specifier: Cow::Borrowed(&specifier),
source_pos: super::DiagnosticSourcePos::SourcePos(pos),
};
Expand Down
34 changes: 14 additions & 20 deletions cli/tests/integration/publish_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,101 +25,95 @@ itest!(missing_deno_json {
args: "publish --token 'sadfasdf'",
output: "publish/missing_deno_json.out",
cwd: Some("publish/missing_deno_json"),
copy_temp_dir: Some("publish/missing_deno_json"),
exit_code: 1,
temp_cwd: true,
});

itest!(invalid_fast_check {
args: "publish --token 'sadfasdf'",
output: "publish/invalid_fast_check.out",
cwd: Some("publish/invalid_fast_check"),
copy_temp_dir: Some("publish/invalid_fast_check"),
exit_code: 1,
temp_cwd: true,
});

itest!(invalid_path {
args: "publish --token 'sadfasdf'",
output: "publish/invalid_path.out",
cwd: Some("publish/invalid_path"),
exit_code: 1,
});

itest!(symlink {
args: "publish --token 'sadfasdf' --dry-run",
output: "publish/symlink.out",
cwd: Some("publish/symlink"),
exit_code: 0,
});

itest!(javascript_missing_decl_file {
args: "publish --token 'sadfasdf'",
output: "publish/javascript_missing_decl_file.out",
cwd: Some("publish/javascript_missing_decl_file"),
copy_temp_dir: Some("publish/javascript_missing_decl_file"),
envs: env_vars_for_registry(),
exit_code: 0,
http_server: true,
temp_cwd: true,
});

itest!(unanalyzable_dynamic_import {
args: "publish --token 'sadfasdf'",
output: "publish/unanalyzable_dynamic_import.out",
cwd: Some("publish/unanalyzable_dynamic_import"),
copy_temp_dir: Some("publish/unanalyzable_dynamic_import"),
envs: env_vars_for_registry(),
exit_code: 0,
http_server: true,
temp_cwd: true,
});

itest!(javascript_decl_file {
args: "publish --token 'sadfasdf'",
output: "publish/javascript_decl_file.out",
cwd: Some("publish/javascript_decl_file"),
copy_temp_dir: Some("publish/javascript_decl_file"),
envs: env_vars_for_registry(),
http_server: true,
exit_code: 0,
temp_cwd: true,
});

itest!(successful {
args: "publish --token 'sadfasdf'",
output: "publish/successful.out",
cwd: Some("publish/successful"),
copy_temp_dir: Some("publish/successful"),
envs: env_vars_for_registry(),
http_server: true,
temp_cwd: true,
});

itest!(config_file_jsonc {
args: "publish --token 'sadfasdf'",
output: "publish/deno_jsonc.out",
cwd: Some("publish/deno_jsonc"),
copy_temp_dir: Some("publish/deno_jsonc"),
envs: env_vars_for_registry(),
http_server: true,
temp_cwd: true,
});

itest!(workspace_all {
args: "publish --token 'sadfasdf'",
output: "publish/workspace.out",
cwd: Some("publish/workspace"),
copy_temp_dir: Some("publish/workspace"),
envs: env_vars_for_registry(),
http_server: true,
temp_cwd: true,
});

itest!(workspace_individual {
args: "publish --token 'sadfasdf'",
output: "publish/workspace_individual.out",
cwd: Some("publish/workspace/bar"),
copy_temp_dir: Some("publish/workspace"),
envs: env_vars_for_registry(),
http_server: true,
temp_cwd: true,
});

itest!(dry_run {
args: "publish --token 'sadfasdf' --dry-run",
cwd: Some("publish/successful"),
copy_temp_dir: Some("publish/successful"),
output: "publish/dry_run.out",
envs: env_vars_for_registry(),
http_server: true,
temp_cwd: true,
});

#[test]
Expand Down
11 changes: 11 additions & 0 deletions cli/tests/testdata/publish/invalid_path.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Checking fast check type graph for errors...
Ensuring type checks...
Check file://[WILDCARD]mod.ts
error[invalid-path]: package path must not contain whitespace (found ' ')
--> [WILDCARD]path with spaces.txt
= hint: rename or remove the file, or add it to 'publish.exclude' in the config file

info: to portably support all platforms, including windows, the allowed characters in package paths are limited
docs: https://jsr.io/go/invalid-path

error: Found 1 problem
7 changes: 7 additions & 0 deletions cli/tests/testdata/publish/invalid_path/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@foo/bar",
"version": "1.0.0",
"exports": {
".": "./mod.ts"
}
}
3 changes: 3 additions & 0 deletions cli/tests/testdata/publish/invalid_path/mod.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function foobar(): string {
return "string";
}
Empty file.
11 changes: 11 additions & 0 deletions cli/tests/testdata/publish/symlink.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Checking fast check type graph for errors...
Ensuring type checks...
Check [WILDCARD]mod.ts
warning[unsupported-file-type]: unsupported file type 'symlink'
--> [WILDCARD]symlink
= hint: remove the file, or add it to 'publish.exclude' in the config file

info: only files and directories are supported
info: the file was ignored and will not be published

Warning Aborting due to --dry-run
7 changes: 7 additions & 0 deletions cli/tests/testdata/publish/symlink/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@foo/bar",
"version": "1.0.0",
"exports": {
".": "./mod.ts"
}
}
3 changes: 3 additions & 0 deletions cli/tests/testdata/publish/symlink/mod.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function foobar(): string {
return "string";
}
1 change: 1 addition & 0 deletions cli/tests/testdata/publish/symlink/symlink
2 changes: 1 addition & 1 deletion cli/tools/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ impl Diagnostic for DocDiagnostic {

fn location(&self) -> DiagnosticLocation {
let specifier = Url::parse(&self.location.filename).unwrap();
DiagnosticLocation::PositionInFile {
DiagnosticLocation::ModulePosition {
specifier: Cow::Owned(specifier),
source_pos: DiagnosticSourcePos::ByteIndex(self.location.byte_index),
}
Expand Down
2 changes: 1 addition & 1 deletion cli/tools/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ impl Diagnostic for LintDiagnostic {

fn location(&self) -> DiagnosticLocation {
let specifier = url::Url::from_file_path(&self.filename).unwrap();
DiagnosticLocation::PositionInFile {
DiagnosticLocation::ModulePosition {
specifier: Cow::Owned(specifier),
source_pos: DiagnosticSourcePos::ByteIndex(self.range.start.byte_index),
}
Expand Down
Loading

0 comments on commit e2ad80b

Please sign in to comment.