Skip to content

Do not add a space after metavariable colon #2549

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

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

topecongiro
Copy link
Contributor

This PR adds two config options space_after_colon_macro_def and space_before_colon_macro_def which control spaces around a colon of metavariable definitions in macro def.

Also this PR changes the default format to not to put a space after a colon. I think that the formatting without a space is more popular in the wild, and incurs less conflicts with users.

Alternatively we could decide whether we should add a space or not based on what kind of separator is used in the definition: if arguments are separated by ,, then we add a space after each colon, but if weird separators, like =>, : or even just a space, are used, then we do not add a space. However this seems too complex and error-prone, so I would rather keep the rule simple and consistent.

@nrc Do you have any thoughts on this?


Do we need two options? I think that we do not need to add space_before_colon_macro_def, I just added it for consistency with other options.

@topecongiro topecongiro force-pushed the macro-def-spaces-around-colon branch from 77cfea0 to cf53ffc Compare March 21, 2018 15:25
@nrc
Copy link
Member

nrc commented Mar 22, 2018

I have thoughts! I think that changing the default is probably right if we don't do anything fancier. However, I think having options for spaces around colons is not the right approach since I think it mostly comes down to which macro we are formatting rather than user preference (i.e., I can imagine the same user wanting to use different formatting on different macros).

We could try to identify $ ident : sequences and format those with the current spacing and not format other colons? I feel like that is the common case and we should be able to format it. I do think that the spacing there should be consistent with function arguments, etc.

@topecongiro topecongiro force-pushed the macro-def-spaces-around-colon branch from cf53ffc to ccec777 Compare March 22, 2018 07:09
@topecongiro
Copy link
Contributor Author

@nrc Thank you for your review! I am convinced that having config options does not do much. I have update this PR so that it only changes the default behavior to not to add a space after colon.

@nrc nrc merged commit 2fbdedb into rust-lang:master Mar 22, 2018
@nrc
Copy link
Member

nrc commented Mar 22, 2018

Thanks!

I've merged the PR, though I worry a little bit about churn for users, if we do try and identify the uses of : for assigning fragment 'types' then we're going to change a lot of code with this PR, then change a lot of code back.

So I think we must decide now, do we:

  • keep this as our long-term formatting
  • implement the heuristic detection (in which case we should do so sooner rather than later)
  • decided we want the heuristics detection but that we can't implement it soon, in which case we should probably revert this PR.

@topecongiro topecongiro deleted the macro-def-spaces-around-colon branch March 23, 2018 11:35
@topecongiro
Copy link
Contributor Author

I prefer to keep the current formatting as a stable way to format macro arguments:

  1. do not put spaces around : of metavariable definition
  2. put a space after : that is not a part of metavariable definition
  3. however, put spaces around : if it directly comes after metavariable

E.g.

macro_rules! foo {
    ($a:ident : $b:ty, bar: $c:ty) => {};
}

@topecongiro
Copy link
Contributor Author

Also we need to get rid of unreachable!()s in macro.rs. I initially thought that macro def like macro foo ($x:) {} will be a parse error, but apparently it is not, and rustfmt will reach an unreacahble code.

@topecongiro topecongiro changed the title Add config option to control spaces around colon in macro def Do not add a space after metavariable colon Mar 25, 2018
@nrc
Copy link
Member

nrc commented Mar 26, 2018

Why $a:ident and not $a: ident? I've seen both in the wild, but the later is consistent with variables and types, or type variables and bounds.

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.

2 participants