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

Prohibit tabs in whitespace check #48709

Merged
merged 2 commits into from
May 4, 2023

Conversation

LilithHafner
Copy link
Member

In general, tabs do not appear in well-formatted Julia source code. This automates that check.

  • Prohibit tabs in whitespace check
  • replace tabs with spaces and fix indentation

Is a superset of and closes #48697

This should possibly also cover other languages, but I don't know what our style conventions are for those languages and they are presumably less important to cover with this check.

This Does Not mean Julia code with tabs will error/warn/etc. It only effects the style enforcement of source code in this repo.

@LilithHafner
Copy link
Member Author

bump

@StefanKarpinski
Copy link
Member

I like this, in fact, so much so that I decided to purge tabs from other kinds of files as well. The code in src/support and src/flisp seems to consistently use tabs rather than spaces and rather than introduce a bunch of noise changing those files, I just excluded them from the check. We can skip whitespace-changing commits when generating blame, but I'm not sure if @JeffBezanson wants me changing all his C code from tabs to spaces. That code is very low churn anyway.

When this gets merged, it would be good to keep at least two commits: one that changes the whitespace and one that changes the check script. We want these separate so the whitespace one can be ignored in blame history. The order of the first two commits should be reversed, however, since the tests will fail after the first commit without the second one.

@LilithHafner LilithHafner added DO NOT MERGE Do not merge this PR! don't squash Don't squash merge labels Mar 3, 2023
@LilithHafner LilithHafner force-pushed the prohibit-tabs-in-whitespace-check branch from fbb66f5 to c8a096f Compare March 3, 2023 22:16
@LilithHafner LilithHafner removed the DO NOT MERGE Do not merge this PR! label Mar 3, 2023
@KristofferC KristofferC force-pushed the prohibit-tabs-in-whitespace-check branch from c8a096f to 36f6826 Compare May 4, 2023 10:35
@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label May 4, 2023
@KristofferC
Copy link
Member

Rebased

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

This looks good to me at this point. Should not be squashed—we want to keep both commits separate!

@LilithHafner LilithHafner changed the title Prohibit tabs Julia source code in whitespace check Prohibit tabs in whitespace check May 4, 2023
@LilithHafner LilithHafner merged commit 93ce36c into master May 4, 2023
@LilithHafner LilithHafner deleted the prohibit-tabs-in-whitespace-check branch May 4, 2023 17:38
@StefanKarpinski
Copy link
Member

@KristofferC, do you want to add the whitespace fix commit to the blameless file?

@KristofferC
Copy link
Member

#49638

@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't squash Don't squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants