Skip to content

Conversation

@wilzbach
Copy link
Contributor

follow-up to #4239

@Hackerpilot
Copy link
Contributor

@JackStouffer
Copy link
Contributor

Why can't we just use dfmt to do this automatically and all in one PR?

@wilzbach
Copy link
Contributor Author

Why can't we just use dfmt to do this automatically and all in one PR?

I think dfmt would even a lot more?
And just adding space already affects 3500 lines, s.t. it's impossible to review via the web interface (I tried).
In any case, I would love to put this style change into one PR ...

@JackStouffer
Copy link
Contributor

I mean have dfmt do everything that it normally does and all in one PR. Assuming there are no bugs in dfmt, this should not be a problem. And even if there were, the auto-tester would catch them.

We would then be able to diff any PR with the dfmt output to stop style deviations.

@wilzbach
Copy link
Contributor Author

I mean have dfmt do everything that it normally does and all in one PR. Assuming there are no bugs in dfmt, this should not be a problem. And even if there were, the auto-tester would catch them.

169 files changed, 97926 insertions(+), 67498 deletions(-)

@JackStouffer
Copy link
Contributor

169 files changed, 97926 insertions(+), 67498 deletions(-)

Better that than hundreds of separate PRs, which is what it would take to enforce all of dfmt's rules. And like I said, the auto-tester will catch any errors.

@Hackerpilot
Copy link
Contributor

I thought that aligning trailing comments was a requirement for using dfmt?

@wilzbach
Copy link
Contributor Author

Better that than hundreds of separate PRs, which is what it would take to enforce all of dfmt's rules. And like I said, the auto-tester will catch any errors.

I just tried. dfmt destroys custom formatting and ddoc headers :/
-> Let's start simple & step by step? I will put all missing operator spaces in this pr.

@wilzbach
Copy link
Contributor Author

I will put all missing operator spaces in this pr.

done.

@JackStouffer
Copy link
Contributor

If this was done automatically using sed, then LGTM

@Hackerpilot Hackerpilot changed the title std.container style fix: space between operators fix space between operators Apr 26, 2016
@Hackerpilot
Copy link
Contributor

Auto-merge toggled on

@tsbockman
Copy link
Contributor

@JackStouffer

the auto-tester will catch any errors.

That's very optimistic of you, for a ~100K line diff. It's not like we have 100% unit test coverage or anything...

@Hackerpilot Hackerpilot merged commit ff298c7 into dlang:master Apr 26, 2016
@wilzbach wilzbach deleted the space_between_operators_containers branch April 26, 2016 21:15
@wilzbach
Copy link
Contributor Author

Thanks :)

@Hackerpilot
Copy link
Contributor

And now I wait for the yelling to start.

@wilzbach
Copy link
Contributor Author

Yep you were quite brave - I didn't expect it to be merged that quickly, but it just touches single lines so I hope for the best!

@JackStouffer
Copy link
Contributor

You avoided the bikeshedding quite skillfully

@yebblies
Copy link
Contributor

I thought that aligning trailing comments was a requirement for using dfmt?

That's for DMD. I'm not sure anyone's really evaluated it for phobos yet - and phobos is not really internally consistent.

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.

5 participants