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(publish): give diagnostic on invalid package files #22082

Merged
merged 2 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading