Skip to content

Commit

Permalink
using warning: set off module and method names
Browse files Browse the repository at this point in the history 
Adds quotes to new warning added in 9c0b9bf.
  • Loading branch information
@pao
pao committed Apr 24, 2015
1 parent 8bcdb3f commit ec99e37
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ static jl_binding_t *jl_get_binding_(jl_module_t *m, jl_sym_t *var, modstack_t *
if (owner != NULL && tempb->owner != b->owner &&
!(tempb->constp && tempb->value && b->constp && b->value == tempb->value)) {
jl_printf(JL_STDERR,
"Warning: both %s and %s export %s; uses of it in module %s must be qualified\n",
"Warning: both `%s` and `%s` export `%s`; uses of it in module `%s` must be qualified\n",
owner->name->name, imp->name->name, var->name, m->name->name);
// mark this binding resolved, to avoid repeating the warning
(void)jl_get_binding_wr(m, var);
Expand Down

6 comments on commit ec99e37

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the sentiment, but this change is not done very well. First of all, there are many messages like this, and now this is the only one that uses quotes. Second, all other messages that quote identifier names and such use double quotes, not backticks. We should use consistent formatting.

Also, is this message really so incomprehensible without quotes?

@pao
Copy link
Member Author

@pao pao commented on ec99e37 Apr 24, 2015

Choose a reason for hiding this comment

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

I found it difficult to read. It is not the only message with quotes like that; there is another one in the output from @IainNZ:

WARNING: Dict-based `@docstring` config is deprecated. Use keywords instead.

Also I just realized that WARNING is capitalized differently in that message than in this one.

Feel free to revert it because it was supposed to be a pull request anyways, and GitHub decided to ignore that and directly commit it.

@pao
Copy link
Member Author

@pao pao commented on ec99e37 Apr 24, 2015

Choose a reason for hiding this comment

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

The garden path reading that led to this, er, forceful request? was

both Digits and Base export contains; uses of it in module Main must be qualified

"Surely, that should be 'contains export'...wait, no, 'contains' is a method name."

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe that other warning is in Base.

Yes, I agree the identifier should be quoted, especially since it's often lowercase.

@pao
Copy link
Member Author

@pao pao commented on ec99e37 Apr 24, 2015

Choose a reason for hiding this comment

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

Roger that.

@pao
Copy link
Member Author

@pao pao commented on ec99e37 Apr 25, 2015

Choose a reason for hiding this comment

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

The other warning is from Docile, it appears.

Please sign in to comment.