search

square / spacecommander

Commit fully-formatted Objective-C as a team without even trying.
Other
1.13k stars 177 forks source link

SortIncludes: true breaks spacecommander? #50

Open vinceis1337 opened 8 years ago

vinceis1337 commented 8 years ago

I'm having an issue where if I have SortIncludes: true in my .clang-format file it will never consider the file properly formatted.

I see in this commit: https://github.com/square/spacecommander/commit/0b388089e7d1309c238a393e747b6126c326ad19

That SortIncludes was set to false in the default .clang-format but we would prefer to continue using it.

Is this just not an option anymore?

alanf commented 8 years ago

Hi @vinceis1337

I remember turning it off because I ran into a strange bug with clang-format. Although I don't remember the exact details, the sort includes behavior was different depending on whether clang-format had the -i (change the file in-place) flag. Space Commander relies on the non-in-place output to detect differences. I just tried it locally and it seemed to work, so I'm not sure what type of input causes the issue.

One thing I've learned about since that you might try: You can pass in a --sort-includes flag to clang-format. So in format-objc-file.sh and format-objc-file-dry-run.sh, you could try adding that flag and see if it works.

Also, could you please provide sample input which triggers the issue? Thanks for the report!

vinceis1337 commented 8 years ago 
#import "RMNThishouldbesecond.h"
#import "RMNAlphabeticallyfirst.h"

I deleted everything I could until it was the bare minimum and that is what I got.

alanf commented 8 years ago

I haven't been able to sort out why the in-place formatting ignores the SortIncludes flag. I would like to use that feature as well.

I may rewrite ./format-objc-file.sh to use stdout and overwrite the file, but it could potentially be slower.

vinceis1337 commented 8 years ago

Hmm, that is pretty odd. Let me know if I can be of any assistance! And thanks for your help!

alanf commented 8 years ago

@vinceis1337 here's a branch that could fix the issue, except it is slower, as predicted (about twice as slow):

https://github.com/square/spacecommander/compare/alanf/50?expand=1

It could be worth it, most of the time, Space Commander will still format a handful of files in under a second, and it would save a lot of time to have it sort the includes for you.

But if one's main use case for Space Commander is to format an entire repo's worth of files, this change could cause considerable pain.