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 EditorConfig file denoting indentation styles #715

Closed
wants to merge 1 commit into from

Conversation

treyhunner
Copy link
Contributor

This .editorconfig file defines the indentation style used in all JavaScript files under src/ and also for the Makefile. Request related to jquery/jquery-ui#606.

I did not add definitions for HTML, JS, or CSS files in the build/ speed/, or test/ directories because the indentation styles were not consistent enough for me to judge how they should be defined.

Below I noted some of the nuances of the indentation styles in the build/ and speed/ directories. The test/ directory also suffers from mixed indentation styles (sometimes in the same file).

build/

tabs:

  • build/sizer.js
  • build/release.js
  • build/release-notes.js
  • build/compile.js
  • build/jshint-check.js
  • build/freq.js
  • build/release-notes.txt

4 spaces:

  • build/lib/jshint.js

8 spaces with 4 spaces for half indentation:

  • build/uglify.js (except for line 7 which uses a 4 column width tabstop)
  • build/lib/parse-js.js
  • build/lib/process.js
  • build/lib/squeeze-more.js

speed/

tabs:

  • speed/css.html
  • speed/event.html
  • speed/slice.vs.concat.html
  • speed/jquery-basis.js (spaces mixed in on line 711)

primarily 2 spaces (with 2 column width tabstops mixed in occasionally):

  • speed/closest.html
  • speed/filter.html
  • speed/find.html
  • speed/index.html
  • speed/benchmarker.js
  • speed/benchmark.js

no idea (heavily mixed spacing and tabbing style)

  • speed/benchmarker.css

If maintaining consistent indentation for the files in these directories is desirable, I suggest that each file's indentation style should be made self-consistent first.

For example something like this may be appropriate for build/:

[build/*.js]
indent_style = tab

[build/uglify.js]
indent_style = space
indent_size = 4

[build/lib/*.js]
indent_style = space
indent_size = 4

@rwaldron
Copy link
Member

This should wait until after we've phased out the existing build system.

@dmethvin
Copy link
Member

@treyhunner, thanks for this pull request. I thought our files were pretty consistent, didn't realize how much variation there was across everything. Once we get the new build system in place we'll be nearly all JavaScript and can apply the core style guidelines consistently across everything. EditorConfig will help us do that for sure.

@rwaldron
Copy link
Member

@dmethvin I also learned of these inconsistencies while building the grunt system - I think it was simply a result of their ad-hoc nature, which we also leave behind

@treyhunner
Copy link
Contributor Author

No problem. If you post back to this request or PM me when the new build system is in place I can update the pull request.

@rwaldron
Copy link
Member

@treyhunner Awesome, thanks!

@dmethvin
Copy link
Member

@treyhunner I think we're ready for your lovin' touch on this. Or at least we're close enough for a first pass. I've created a ticket to track this so it will be in the release notes: http://bugs.jquery.com/ticket/11777

@treyhunner
Copy link
Contributor Author

@dmethvin It looks better than before. I updated the .editorconfig file with some explicit rules for specific files and groups of files. The test/ directory is still a somewhat inconsistent, and some other files have some minor issues as well. I will be submitting a few indentation corrections soon in a separate pull request.

build/*.js

All build/*.js files use tabs for indentation except for build/uglify.js which uses 8 spaces for indentation with the exception of some deeply nested code.

build/lib/*.js

The following build/lib functions use 8 spaces for indentation overal, build/lib/parse-js.js, build/lib/process.js, build/lib/squeeze-more.js.

The exceptions to the 8 space indentation used in those files include:

The file build/lib/jshint.js uses 4 spaces for indentation

test/

It looks like the .html and .php files in test/ use tabs for indentation, but many of them use occasional spaces for indentation. The JavaScript files use tabs for indentation with occasional spaces mixed in as well. The .xml files use tabs consistently and the single .css file uses 8 spaces.

test/unit/*.js

The test/unit/core.js files all use tabs for indentation except for test/unit/core.js and test/unit/ajax.js which use mostly tabs but have mixed tab/space indentation mixed in as well.

@rwaldron
Copy link
Member

The build/* dir is going away.

@treyhunner
Copy link
Contributor Author

@rwldrn updated pull request to remove build/ from .editorconfig.

@dmethvin
Copy link
Member

There are still some tweaks that should be done here, but I figured it would be easier to spot and fix them once the file landed in master. Thanks @treyhunner!

markelog pushed a commit to markelog/jquery that referenced this pull request Jun 29, 2012
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants