-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
warning cli: tweak single-letter warning deprecation #10312
Changes from 4 commits
7921b9a
8518de1
1e4a42c
1f4c469
478299a
47c38f2
8f5482d
c21fa45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -497,8 +497,7 @@ type token = | |
| Num of int * int * modifier | ||
|
||
let letter_alert tokens = | ||
let deprecated = function Letter (c,None) -> Some c | _ -> None in | ||
let print_char ppf c = | ||
let print_warning_char ppf c = | ||
let lowercase = Char.lowercase_ascii c = c in | ||
Format.fprintf ppf "%c%c" | ||
(if lowercase then '-' else '+') c | ||
|
@@ -514,15 +513,35 @@ let letter_alert tokens = | |
else | ||
Format.fprintf ppf "%a%d..%d" print_modifier m a b | ||
| Letter(l,Some m) -> Format.fprintf ppf "%a%c" print_modifier m l | ||
| Letter(l,None) -> print_char ppf l | ||
| Letter(l,None) -> print_warning_char ppf l | ||
in | ||
match List.find_map deprecated tokens with | ||
| None -> None | ||
| Some example -> | ||
let consecutive_letters = | ||
(* we are tracking consecutive unsigned letter in warning strings: | ||
for instance in '-w "not-principa"'. *) | ||
let commit_chunk l = function | ||
| [] | [ _ ] -> l | ||
| _ :: _ :: _ as chunk -> List.rev chunk :: l | ||
in | ||
let group_consecutive_letters (l,current) = function | ||
| Letter (x, None) -> (l, x::current) | ||
| _ -> (commit_chunk l current, []) | ||
in | ||
let l, on_going = | ||
List.fold_left group_consecutive_letters ([],[]) tokens | ||
in | ||
commit_chunk l on_going | ||
in | ||
match consecutive_letters with | ||
| [] | [] :: _ -> None | ||
| example :: _ -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not fond of the
So I would be in favor of dropping that subcase completely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, this was a remnant of the previous iteration on the warning message. |
||
let pos = { Lexing.dummy_pos with pos_fname = "_none_" } in | ||
let nowhere = { loc_start=pos; loc_end=pos; loc_ghost=true } in | ||
let spelling_hint ppf = | ||
if List.length (List.filter_map deprecated tokens) >= 5 then | ||
let max_seq_len = | ||
List.fold_left (fun l x -> max l (List.length x)) | ||
0 consecutive_letters | ||
in | ||
if max_seq_len >= 5 then | ||
Format.fprintf ppf | ||
"@ @[Hint: Did you make a spelling mistake \ | ||
when using a mnemonic name?@]" | ||
|
@@ -531,14 +550,13 @@ let letter_alert tokens = | |
in | ||
let message = | ||
Format.asprintf | ||
"@[<v>@[Setting a warning with single lowercase \ | ||
or uppercase letters, like '%c' or '%c',@ is deprecated.@]@ \ | ||
"@[<v>@[Setting a warning with a sequence of lowercase \ | ||
or uppercase letters,@ like '%a',@ is deprecated.@]@ \ | ||
@[Use the equivalent signed form:@ %t.@]@ \ | ||
@[Hint: Enabling or disabling a warning by its mnemonic name \ | ||
requires a + or - prefix.@]\ | ||
%t@?@]" | ||
(Char.lowercase_ascii example) | ||
(Char.uppercase_ascii example) | ||
Format.(pp_print_list ~pp_sep:(fun _ -> ignore) pp_print_char) example | ||
(fun ppf -> List.iter (print_token ppf) tokens) | ||
spelling_hint | ||
in | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for the late-night nitpick (somewhere on earth!), but you never explicitly say that we are considering at-least-2 consecutive letters. Maybe that would be worth pointing out in the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a single letter still consecutive with itself? Anyway, I will disambiguate this comment and add some tests tomorrow (somewhere on Earth).