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

Add invalid doc comment help message #36964

Merged
merged 1 commit into from
Oct 19, 2016

Conversation

GuillaumeGomez
Copy link
Member

Fixes #36946.

Any opinion on the message?

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -792,7 +792,7 @@ pub enum GateIssue {
}

pub fn emit_feature_err(sess: &ParseSess, feature: &str, span: Span, issue: GateIssue,
explain: &str) {
explain: &str, help_note: Option<&str>) {
Copy link
Member

@solson solson Oct 4, 2016

Choose a reason for hiding this comment

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

I would instead make a version of this function (say, feature_err) that returns the err instead of calling err.emit(), and have this function just do feature_err(...).emit(). Then the existing callers of this function don't need to change for the None argument, and the stmt_expr_attributes one can do let err = feature_err(...); err.help(...); err.emit();.

@@ -161,7 +161,8 @@ impl<'a> StripUnconfigured<'a> {
"stmt_expr_attributes",
attr.span,
GateIssue::Language,
EXPLAIN_STMT_ATTR_SYNTAX);
EXPLAIN_STMT_ATTR_SYNTAX,
Some("If you just want to comment your code, just use '//'"));
Copy link
Member

Choose a reason for hiding this comment

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

I would say something like

`///` is for documentation comments. For a plain comment, use `//`.

Copy link
Member

Choose a reason for hiding this comment

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

Won't this help trigger for all misplaced attributes? It should only trigger for misplaced doc attributes.

@bors
Copy link
Contributor

bors commented Oct 7, 2016

☔ The latest upstream changes (presumably #36945) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Updated.

Copy link
Member

@solson solson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@GuillaumeGomez
Copy link
Member Author

We now need a reviewer. cc @jonathandturner :p

attr.span,
GateIssue::Language,
EXPLAIN_STMT_ATTR_SYNTAX)
.help("`///` is for documentation comments. For a plain comment, use `//`.")
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I forgot to bring this up again.

This help should only be generated for misplaced doc comments, but this will generate it for any misplaced attributes at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

@solson - sounds like an interesting point. @GuillaumeGomez ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't think of testing in other cases. Good point! I'll check a bit later.

}

pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: GateIssue,
explain: &str) -> DiagnosticBuilder<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: indentation is off here

@GuillaumeGomez
Copy link
Member Author

Updated.

@sophiajt
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 18, 2016

📌 Commit 93417fa has been approved by jonathandturner

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016
…athandturner

Add invalid doc comment help message

Fixes rust-lang#36946.

Any opinion on the message?
eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016
…athandturner

Add invalid doc comment help message

Fixes rust-lang#36946.

Any opinion on the message?
bors added a commit that referenced this pull request Oct 19, 2016
@bors bors merged commit 93417fa into rust-lang:master Oct 19, 2016
@GuillaumeGomez GuillaumeGomez deleted the comment_error branch October 19, 2016 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants