Suggest && and || instead of 'and' and 'or' by vi · Pull Request #54181 · rust-lang/rust

vi · 2018-09-13T13:05:45Z

Resolves #54109.

Note: competing pull reqeust: #54179

r? @csmoe

Closes rust-lang#54109.

csmoe · 2018-09-13T13:09:42Z

cc @estebank I prefer to this PR than mine.

estebank

This is great! One small piece of feedback left. r=me after dealing with it.

Thank you @csmoe too for your change.

estebank · 2018-09-13T16:43:35Z

src/libsyntax/parse/parser.rs

            };
            let mut err = self.fatal(&msg_exp);
+            if self.token.is_ident_named("and") {
+                err.help("Use `&&` instead of `and` for the boolean operator");


Could you use the following?

err.span_suggestion_with_applicability( self.span, "use `&&` instead of `and` for the boolean operator", "&&".to_string(), Applicability::MaybeIncorrect, );

This way the output will be presented inline and third party tools will be able to apply the suggestion in their UIs.

Same change in all four cases.

@estebank

Advised by @estebank.

vi · 2018-09-13T17:53:16Z

Appled the recommendation. Also added while tests in addition to existing if tests just in case.

Is it not a problem that output looks a bit repetitive?:

help: use `||` instead of `or` for the boolean operator: `||`

estebank · 2018-09-13T18:39:19Z

There's a short span suggestion method that hides the actual suggestion and would work well for these, but it doesn't accept Applicability... I'm ok with it for now, but if you're in the mood to check it out to see if we could make it accept Applicability and modify every caller, it would be great :)

estebank · 2018-09-14T17:57:19Z

@bors r+ rollup
@vi If you get to update span_suggestion_short before this PR is merged, great, otherwise it can be a follow up task.

bors · 2018-09-14T17:57:20Z

📌 Commit 79919a7 has been approved by estebank

vi · 2018-09-14T20:51:35Z

As far as I understoon from the description, it is expected to change not directly related code ("modify every caller"), so another review may be needed.

Is accepting Applicability to span_suggestion_short useful for other lints, not for just this?

estebank · 2018-09-14T21:08:09Z

@vi we should always be specifying Applicability. We don't as an accident of history: no one has bothered to go back and fully fix it everywhere.

vi · 2018-09-14T23:06:07Z

There is already span_suggestion_short_with_applicability and it's more used than span_suggestion_short.

What Applicability should have the remaining span_suggestion_shorts?

Applied the span_suggestion_short_with_applicability for now.

estebank · 2018-09-14T23:27:59Z

@bors r+

bors · 2018-09-14T23:28:00Z

📌 Commit bc63a4a has been approved by estebank

vi · 2018-09-15T00:23:04Z

Also tried replacing all span_suggestion[_short] with span_suggestion[_short]_with_applicability of Applicability::Unspecified.

Shall I just push it or submit another pull reqeust? Shall abandoned functions be deprecated?

estebank · 2018-09-15T01:18:20Z

@vi that change would be great as a stand alone PR. Remember to fix indentation when needed and changing the stderr files.

vi · 2018-09-15T01:27:24Z

fix indentation

cargo fmt -- --check shows a lot of unrelated changes and I'm not sure what is correct indentation.

changing the stderr files

Assuming the change touches tests?

@csmoe

Suggest && and || instead of 'and' and 'or' Resolves rust-lang#54109. Note: competing pull reqeust: rust-lang#54179 r? @csmoe

@ghost

Rollup of 5 pull requests Successful merges: - #53941 (rustdoc: Sort implementors) - #54181 (Suggest && and || instead of 'and' and 'or') - #54209 (Partially revert 674a5db "Fix undesirable fallout [from macro modularization]") - #54213 (De-overlap the lifetimes of `flow_inits` and `flow_{un,ever_}inits`.) - #54244 (Add a small search box to seach Rust's standary library) Failed merges: r? @ghost

estebank · 2018-09-16T22:37:52Z

fix indentation

cargo fmt -- --check shows a lot of unrelated changes and I'm not sure what is correct indentation.

I believe I was referring to making sure that when a line is above 100 cols, you change it as you did in this PR:

	        e.span_suggestion_short_with_applicability(
                    self.span,
                    "use `&&` instead of `and` for the boolean operator",
                    "&&".to_string(),
                    Applicability::MaybeIncorrect,
                );

Assuming the change touches tests?

Correct :)

berkus · 2018-09-19T10:42:19Z

errrr... why?

how is && or || more readable and understandable than and / or? Apart from everything else I'd rather not see &&/|| in the code at all. Even for C++ I use and/or as human-readable equivalents of the pseudographic garbage.

vi · 2018-09-19T11:36:44Z

@berkus I think Rust is just based on C in that aspect. C has &&, so Rust has &&. Haskell also have a convention that operators are special characters and functions are letters. This pull request does not set or enforce &&/||, it just specifically hints them for users who used and/or by mistake.

If in future some edition of Rust decides to implement both and and && (like in Perl), or just the former like in Python, this suggestion will be modified accordingly.

berkus · 2018-09-21T19:42:58Z

I agree that linter is not to blame here, but where and how can I suggest the more readable and/or alternatives for inclusion/use in the language?

estebank · 2018-09-21T20:08:49Z

@berkus submit an RFCS.

vi · 2018-09-21T20:55:03Z

I also like Perl-style && and and mix. But it's too Perl.

vi added 2 commits September 13, 2018 15:54

Suggest && and || instead of 'and' and 'or'

636f518

Closes rust-lang#54109.

Add tests for issue 54109

888b8c9

rust-highfive assigned csmoe Sep 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2018

vi mentioned this pull request Sep 13, 2018

Add lint message for unexpected and/or in if statement #54179

Closed

estebank approved these changes Sep 13, 2018

View reviewed changes

vi added 2 commits September 13, 2018 20:44

Use span_suggestion_with_applicability for "and/or" hinter

acc44e4

Advised by @estebank.

Add "while" tests for issue 54109

79919a7

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2018

issue 54109: use short suggestions

bc63a4a

GuillaumeGomez mentioned this pull request Sep 16, 2018

Rollup of 5 pull requests #54270

Merged

bors merged commit bc63a4a into rust-lang:master Sep 16, 2018

Uh oh!

Conversation

vi commented Sep 13, 2018

Uh oh!

csmoe commented Sep 13, 2018

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

estebank Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

vi commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Sep 13, 2018

Uh oh!

estebank commented Sep 14, 2018

Uh oh!

bors commented Sep 14, 2018

Uh oh!

vi commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Sep 14, 2018

Uh oh!

vi commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Sep 14, 2018

Uh oh!

bors commented Sep 14, 2018

Uh oh!

vi commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Sep 15, 2018

Uh oh!

vi commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Sep 16, 2018

Uh oh!

berkus commented Sep 19, 2018

Uh oh!

vi commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkus commented Sep 21, 2018

Uh oh!

estebank commented Sep 21, 2018

Uh oh!

vi commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vi commented Sep 13, 2018 •

edited

Loading

vi commented Sep 14, 2018 •

edited

Loading

vi commented Sep 14, 2018 •

edited

Loading

vi commented Sep 15, 2018 •

edited

Loading

vi commented Sep 15, 2018 •

edited

Loading

vi commented Sep 19, 2018 •

edited

Loading