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 a new CI test to make sure we use tabs and not spaces. Fixes #2050 #2105

Merged
merged 4 commits into from Dec 14, 2015
Merged

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 23, 2015

So, based on Etienne's recent PR on tabs/spaces, and after watching this video where a GitHub guy was saying they're now using CI for much more than just tests (CSS validation etc. - watch the video!) I thought it would be interesting if we did the same.

I've butchered Etienne's gist ruby script (and probably written very bad ruby code) and have attempted to write a script that will check all newly added lines in .m/.h files and check to make sure they start with tabs and not spaces

@pjrobertson pjrobertson force-pushed the ci branch 2 times, most recently from 07d6c3b to 714cf06 Compare Sep 23, 2015
pjrobertson added a commit that referenced this issue Sep 23, 2015
This reverts commit 34de8cd.
Fixes a space-added line (for testing of #2105)
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 23, 2015

P.S. I know @skurfer was against using 'custom' scripts originally, but maybe I can persuade him here... ;-)

There's definitely scope for us to expand this out in the future - adding in more scripts and stuff, and it's not exactly 'difficult' to remember (although maybe we should have a directory for travis stuff kept separate).
Perhaps some day we could even have some kind of 'real' CI that meant when plugin PRs are merged they are automatically built and pushed to the plugin repo :-)

@tiennou
Copy link
Member

@tiennou tiennou commented Sep 23, 2015

God, I feel like the pesky guy ;-). I'm not too fond of dot-hiding the script, could it be moved under /Scripts (or wherever we have those, I don't remember the name offhand) ?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 23, 2015

...probably a good idea :-)
I've given it its own folder now, and have removed my test commits. It works as expected (fails when there are lines with spaces) so that's all good :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 23, 2015

Man, you guys are really going ahead with this tab stuff, eh?

  • Why is the Map stuff in here? It should already be on master.
  • I was against using a custom script to do something Travis should handle natively. This doesn’t bother me too much.
  • My first thought was “What a bullshit reason to fail tests!” but if you were forcing spaces instead of tabs, I’d think “Oh, yeah. We need that!” so… my argument is probably not objective. 😀
  • I see the new Ruby script is indented using spaces, which is hilarious. At least it shows we’re not complete animals. 🐗
  • Do you want to say that this “fixes” #2050?

@pjrobertson pjrobertson changed the title Add a new CI test to make sure we use tabs and not spaces Add a new CI test to make sure we use tabs and not spaces. Fixes #2050 Sep 23, 2015
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 23, 2015

Why is the Map stuff in here? It should already be on master.

No reason except for sloppy git skills. Fixed

I was against using a custom script to do something Travis should handle natively. This doesn’t bother me too much.

Cool

My first thought was “What a bullshit reason to fail tests!” but if you were forcing spaces instead of tabs, I’d think “Oh, yeah. We need that!” so… my argument is probably not objective. 😀

Personally I'm still pro spaces, but since the QS codebase is primarily tabs as Etienne's discovered, I thought it was best we go down that route

I see the new Ruby script is indented using spaces, which is hilarious. At least it shows we’re not complete animals. 🐗

Hahaha, well that's just hilarious! Maybe we need to decide what 'coding style' we have for ruby though... what if we want to use spaces? ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 25, 2015

Better. Were the tests not getting called via xctool before? I thought they were running somehow.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 28, 2015

Better. Were the tests not getting called via xctool before? I thought they were running somehow.

I basically copied the exact same command that Travis was calling before I made these changes.
If you look at the logs for tests on this branch, you'll see exactly the same thing is run/log output is generated.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 28, 2015

...I've also switched check_indent to use tabs...!

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 15, 2015

Can this be merged? Every day that passes, we run the risk of ruining our code with spaces.. ;-)

skurfer added a commit that referenced this issue Dec 14, 2015
Add a new CI test to make sure we use tabs and not spaces. Fixes #2050
@skurfer skurfer merged commit 6af9673 into master Dec 14, 2015
2 checks passed
@skurfer skurfer deleted the ci branch Dec 14, 2015
skurfer added a commit that referenced this issue Dec 14, 2015
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.

None yet

3 participants