Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

Added "textblock" value to requireCapitalizedComments #769

Closed
wants to merge 7 commits into from

Conversation

indexzero
Copy link
Contributor

This makes the most sense to me right now. If you're good with this implementation then I'll add the docs to this PR as well. Fixes #767.

@coveralls
Copy link

Coverage Status

Coverage decreased (-93.2%) when pulling d9d7e1d on indexzero:gh-767 into 32da403 on jscs-dev:master.

@indexzero
Copy link
Contributor Author

I don't understand the coveralls message, but happy to fix it.

@@ -1,34 +1,92 @@
var Checker = require('../../lib/checker');
var assert = require('assert');

describe('rules/require-capitalized-comments', function() {
describe.only('rules/require-capitalized-comments', function() {
Copy link
Member

Choose a reason for hiding this comment

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

oh it's probably the .only

Copy link
Member

Choose a reason for hiding this comment

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

mocha allows you to filter tests: mocha debug -b -g 'require-capitalized-comments'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! I forgot to remove that.

@mikesherov
Copy link
Contributor

Good work! I don't think we need a new option value here. @mrjoelkemp already stated that he considers this a bug.

@indexzero
Copy link
Contributor Author

@mikesherov ok. I'll remove the check for it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 9703f96 on indexzero:gh-767 into 32da403 on jscs-dev:master.

@indexzero
Copy link
Contributor Author

💯

].join('\n')).getErrorCount() === 1);
});

it('should report on multiple uppercase lines in multiple "textblocks"', function() {
Copy link
Member

Choose a reason for hiding this comment

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

should not report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Typo

@mrjoelkemp
Copy link
Member

Really interesting test cases @indexzero. Great job on this. I'll take a closer look through the implementation and edge cases tonight (hopefully).

@indexzero
Copy link
Contributor Author

@mrjoelkemp thanks. I actually just made a small change to support sentence breaks in textblocks which are perfectly valid.

//
// For example I often end my sentences on a single line.
// And then start the next one on the following line.
//

Actually checking for . in the previous line crossed a boundary with linting into grammar checking that I didn't want to cross.

var checker;

// Remark (indexzero): Test helper which makes multi-line comment testing look cleaner
Copy link
Member

Choose a reason for hiding this comment

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

Just an fyi, we typically don't include the name of the committer in the code; that info could be found via git blame. I'll remove post-merge.

It's also pretty clear that the helper cleans things up a bit.

@mrjoelkemp
Copy link
Member

@indexzero this looks great. thanks for contributing!

I'll fix the travis fail; be sure to run npm test locally before pushing up to catch the style errors on your PR.

It would be great for parity if you could also enhance disallowCapitalizedComments to fail on the cases where requireCapitalizedComments passes. We'd greatly appreciate the help. A follow up PR works.

@mrjoelkemp
Copy link
Member

@indexzero Filed an issue for disallowCapitalizedComments. #775

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

requireCapitalizedComments and multi-line "line" comments
6 participants