Conversation
| } | ||
|
|
||
| /// For warnings/errors which have a ReportMode that dictates when they are reported | ||
| pub trait Reportable { |
There was a problem hiding this comment.
This trait name might be confusing, bikesheds are welcome
There was a problem hiding this comment.
Warn ? We only use it for Warnings specifically at the moment so no need to be overly general, right?
But with the module name as context not terrible
There was a problem hiding this comment.
I think it's not really accurate to the purpose of the trait since the trait doesn't really have anything to do with warnings specifically, it just grabs the report mode
sholderbach
left a comment
There was a problem hiding this comment.
Good move in general, minor grumpiness about the pub impl details.
| /// A bloom-filter like structure to store the hashes of warnings, | ||
| /// without actually permanently storing the entire warning in memory. | ||
| /// May rarely result in warnings incorrectly being unreported upon hash collision. | ||
| #[derive(Default)] | ||
| pub struct ReportLog { | ||
| // A bloom-filter like structure to store the hashes of `ParseWarning`s, | ||
| // without actually permanently storing the entire warning in memory. | ||
| // May rarely result in warnings incorrectly being unreported upon hash collision. | ||
| parse_warnings: Vec<u64>, | ||
| } | ||
| pub struct ReportLog(Vec<u64>); |
There was a problem hiding this comment.
I dislike pub tuple structs in general and even more when their internals are non obvious.
| } | ||
|
|
||
| /// For warnings/errors which have a ReportMode that dictates when they are reported | ||
| pub trait Reportable { |
There was a problem hiding this comment.
Warn ? We only use it for Warnings specifically at the moment so no need to be overly general, right?
But with the module name as context not terrible
| 4. Do you want to report a warning but not stop execution? | ||
| - **NEVER** `println!`, we can write to stderr if necessary but... | ||
| - good practice: `nu_protocol::cli_error::report_error` or `report_error_new` | ||
| - good practice: `nu_protocol::report_error::report_error` or `report_error_new` |
There was a problem hiding this comment.
Thanks for picking this up.
| impl Hash for ShellWarning { | ||
| fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||
| match self { | ||
| ShellWarning::Deprecated { | ||
| dep_type, label, .. | ||
| } => { | ||
| dep_type.hash(state); | ||
| label.hash(state); |
There was a problem hiding this comment.
Future spitballing:
From the perspecitve of the ReportMode using a single Hash that ignores the span is suboptimal if we want to report all distinct uses but only once for each execution. (which would be the behavior like LSP warnings/errors)
There was a problem hiding this comment.
It actually already does report each instance in LSP already since that reads parse errors/warnings directly and doesn't go through the normal report_parse_warning that CLI errors do
There was a problem hiding this comment.
that being said sometimes it would be nice to have some way to show all instances if you're trying to audit deprecation for example but it definitely shouldn't be the default imo because we don't want to overwhelm people with deprecation warnings
|
Not caused by this PR, I noticed while doing some ad-hoc testing that we can only generate one warning per source file. Sourcing the following file (or entering it in the REPL) only generates a warning for {} | get --sensitive a?
[] | filter {|| }Sourcing it again doesn't report a second warning for I can only get a warning for |
…at (#16151) # Description This PR depends on #16147, use `git diff 132ikl/shell-warning 132ikl/isolation-warn` to see only changes from this PR People seem to get tripped up by this a lot, and it's not exactly intuitive, so I added a warning if you try to set `$env.config.history.isolation = true` when using the plaintext file format: Warning: nu::shell::invalid_config ⚠ Encountered 1 warnings(s) when updating config Warning: nu::shell::incompatible_options ⚠ Incompatible options ╭─[source:1:33] 1 │ $env.config.history.isolation = true · ──┬─ · ╰── history isolation only compatible with SQLite format ╰──── help: disable history isolation, or set $env.config.history.file_format = "sqlite" # User-Facing Changes * Added a warning when using history isolation without using SQLite history. # Tests + Formatting Added a test
# Description Adds a proper `ShellWarning` enum which has the same functionality as `ParseWarning`. Also moves the deprecation from nushell#15806 into `ShellWarning::Deprecated` with `ReportMode::FirstUse`, so that warning will only pop up once now. # User-Facing Changes Technically the change to the deprecation warning from nushell#15806 is user facing but it's really not worth listing in the changelog
…at (nushell#16151) # Description This PR depends on nushell#16147, use `git diff 132ikl/shell-warning 132ikl/isolation-warn` to see only changes from this PR People seem to get tripped up by this a lot, and it's not exactly intuitive, so I added a warning if you try to set `$env.config.history.isolation = true` when using the plaintext file format: Warning: nu::shell::invalid_config ⚠ Encountered 1 warnings(s) when updating config Warning: nu::shell::incompatible_options ⚠ Incompatible options ╭─[source:1:33] 1 │ $env.config.history.isolation = true · ──┬─ · ╰── history isolation only compatible with SQLite format ╰──── help: disable history isolation, or set $env.config.history.file_format = "sqlite" # User-Facing Changes * Added a warning when using history isolation without using SQLite history. # Tests + Formatting Added a test
Description
Depends on #16146, do
git diff 132ikl/warning-diagnostic 132ikl/shell-warningto see changes only from this PR.Adds a proper
ShellWarningenum which has the same functionality asParseWarning.Also moves the deprecation from #15806 into
ShellWarning::DeprecatedwithReportMode::FirstUse, so that warning will only pop up once now.User-Facing Changes
Technically the change to the deprecation warning from #15806 is user facing but it's really not worth listing in the changelog
Tests + Formatting
N/A
After Submitting
N/A