Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#26018 closed defect (bug) (fixed)

Coding standards / whitespace for wp-admin/js/word-count.js

Reported by: dougwollison's profile dougwollison Owned by:
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Editor Keywords:
Focuses: Cc:

Description

Patch to come

Attachments (2)

26018.diff (2.0 KB) - added by dougwollison 10 years ago.
Initial changes: spacing, braces, equals, keys fixed and globals declared. Also fixed what looks like a syntax error with one of the regexs.
26018.2.diff (566 bytes) - added by jorbin 10 years ago.

Download all attachments as: .zip

Change History (8)

@dougwollison
10 years ago

Initial changes: spacing, braces, equals, keys fixed and globals declared. Also fixed what looks like a syntax error with one of the regexs.

@jorbin
10 years ago

#1 follow-up: @jorbin
10 years ago

Thanks for the patch. A couple of concerns.

If we want to change one of the regexs, we should ideally do it through it's own tickets and create unit tests as a part of that. That specific regex has been there for 3 years (since [17936] )

All the formatting changes make the svn history much harder to follow for this file. While that is a concern that these jshint changes themselves are going to do that as well, the benefits of having a passing jshint for preventing bugs and enforcing many parts of our standard out ways this cost. To quote @nacin as we do in our style guide, “Code refactoring should not be done just because we can.”. I think it's fine on lines we are changing anyways and in general that is the best way for us to go about cleaning up our files.

I've submitted an updated patch with just the jshint related changes

#2 @nacin
10 years ago

  • Summary changed from jshint shouldn't throw errors: wp-admin/js/word-count.js to Coding standards / whitespace for wp-admin/js/word-count.js

I ended up fixing word-count.js in [26209] because I thought it wasn't claimed (accounting error).

Renaming this ticket to reflect whitespace changes.

#3 in reply to: ↑ 1 @dougwollison
10 years ago

Replying to jorbin:

Thanks for the patch. A couple of concerns.

If we want to change one of the regexs, we should ideally do it through it's own tickets and create unit tests as a part of that. That specific regex has been there for 3 years (since [17936] )

All the formatting changes make the svn history much harder to follow for this file. While that is a concern that these jshint changes themselves are going to do that as well, the benefits of having a passing jshint for preventing bugs and enforcing many parts of our standard out ways this cost. To quote @nacin as we do in our style guide, “Code refactoring should not be done just because we can.”. I think it's fine on lines we are changing anyways and in general that is the best way for us to go about cleaning up our files.

I've submitted an updated patch with just the jshint related changes

You're right; if the regex works then ignore that change and use yours. I only noticed it anyway because it was screwing with my editor's syntax highlighting for the inline comment on that line.

#4 @wonderboymusic
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

In [26209]

#5 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 3.8

#6 @SergeyBiryukov
10 years ago

  • Component changed from Build/Test Tools to Editor
Note: See TracTickets for help on using tickets.