Skip to content

Improve position named arguments lint underline and formatting names - #99958

Merged
bors merged 2 commits into
rust-lang:masterfrom
PrestonFrom:issue_99907
Aug 3, 2022
Merged

Improve position named arguments lint underline and formatting names#99958
bors merged 2 commits into
rust-lang:masterfrom
PrestonFrom:issue_99907

Conversation

@PrestonFrom

@PrestonFromPrestonFrom commented Jul 30, 2022

Copy link
Copy Markdown
Contributor

For named arguments used as implicit position arguments, underline both
the opening curly brace and either:

  • if there is formatting, the next character (which will either be the
    closing curl brace or the : denoting the start of formatting args)
  • if there is no formatting, the entire arg span (important if there is
    whitespace like { })

This should make it more obvious where the named argument should be.

Additionally, in the lint message, emit the formatting argument names
without a dollar sign to avoid potentially confusion.

Fixes#99907

@rustbotrustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 30, 2022
@rust-highfive

rust-highfive commented Jul 30, 2022

Copy link
Copy Markdown
Contributor

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2022
@PrestonFrom

PrestonFrom commented Jul 30, 2022

Copy link
Copy Markdown
ContributorAuthor

@lqd This addresses your comment here: #99660 (comment)

Not sure if you want to be the reviewer here or not though?

@PrestonFromPrestonFrom mentioned this pull request Jul 30, 2022
Merged
@compiler-errors

compiler-errors commented Aug 2, 2022

Copy link
Copy Markdown
Contributor

r? @compiler-errors since i reviewed the other two PRs touching this code

compiler-errors
compiler-errors reviewed Aug 2, 2022
View reviewed changes
Comment threadsrc/test/ui/macros/issue-99265.stderr Outdated

@compiler-errorscompiler-errorsAug 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to drop "only" here?

@PrestonFromPrestonFromAug 2, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous PR, the lint will trigger for every case a named argument is used positionally even if it's used by name elsewhere, so I think "only" is now incorrect. (I missed this in the last change.)

@compiler-errorscompiler-errorsAug 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok.

compiler-errors
compiler-errors reviewed Aug 2, 2022
View reviewed changes
Comment threadsrc/test/ui/macros/issue-99265.stderr Outdated
compiler-errors
compiler-errors reviewed Aug 2, 2022
View reviewed changes
Comment threadcompiler/rustc_builtin_macros/src/format.rs Outdated
compiler-errors
compiler-errors reviewed Aug 2, 2022
View reviewed changes
Comment threadcompiler/rustc_builtin_macros/src/format.rs Outdated
@PrestonFrom
Improve position named arguments lint underline and formatting names
d0ea440
For named arguments used as implicit position arguments, underline both
the opening curly brace and either:
* if there is formatting, the next character (which will either be the
closing curl brace or the `:` denoting the start of formatting args)
* if there is no formatting, the entire arg span (important if there is
whitespace like `{ }`)
This should make it more obvious where the named argument should be.
Additionally, in the lint message, emit the formatting argument names
without a dollar sign to avoid potentially confusion.
Fixesrust-lang#99907
@PrestonFrom
Move if-block into closure to reduce duplicate code
298acef
@PrestonFrom

PrestonFrom commented Aug 2, 2022

Copy link
Copy Markdown
ContributorAuthor

@compiler-errors Thank you for the review!

It looks like #99987 is still in the queue but at the moment I don't think it should have any merge conflicts.

@compiler-errors

compiler-errors commented Aug 2, 2022

Copy link
Copy Markdown
Contributor

Alright, seems like it didn't conflict.

@bors r+

@bors

bors commented Aug 2, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 298acef has been approved by compiler-errors

It is now in the queue for this repository.

@borsbors 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 Aug 2, 2022
@matthiaskrgrmatthiaskrgr mentioned this pull request Aug 2, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2022
@bors
Auto merge ofrust-lang#100082- matthiaskrgr:rollup-ywu4iux, r=matth…
e141246
…iaskrgr
Rollup of 6 pull requests
Successful merges:
- rust-lang#99933 (parallelize HTML checking tool)
- rust-lang#99958 (Improve position named arguments lint underline and formatting names)
- rust-lang#100008 (Update all pre-cloned submodules on startup)
- rust-lang#100049 (:arrow_up: rust-analyzer)
- rust-lang#100070 (Clarify Cargo.toml comments)
- rust-lang#100074 (rustc-docs: Be less specific about the representation of `+bundle`)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 82feb49 into rust-lang:masterAug 3, 2022
@rustbotrustbot added this to the 1.64.0 milestone Aug 3, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@compiler-errorscompiler-errorscompiler-errors left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@compiler-errorscompiler-errors

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.64.0

Development

Successfully merging this pull request may close these issues.

For position named arguments lint, underline both curly brackets when inserting and don't add $ to formatting names in messages

6 participants

@PrestonFrom@rust-highfive@compiler-errors@bors@michaelwoerister@rustbot