Skip to content
This repository has been archived by the owner. It is now read-only.

requireSpaceAfterLineComment should skip msjsdoc comments #595

Conversation

@zxqfox
Copy link
Member

zxqfox commented Aug 27, 2014

closes #593

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 27, 2014

Coverage Status

Coverage increased (+0.01%) when pulling 6d6042f on zxqfox:hotfix/require-space-after-comment-line-triple-slashed into 2fa147c on jscs-dev:master.

@avishnyak

This comment has been minimized.

Copy link

avishnyak commented Aug 27, 2014

There is one issue with the rule as implemented right now. What is your opinion on the following case which is valid msjsdoc format?

    var jQuery = function (selector, context) {
        /// <summary>
        ///     1: Accepts a string containing a CSS selector which is then used to match a set of elements.
        ///     &#10;    1.1 - $(selector, context) 
        ///     &#10;    1.2 - $(element) 
        ///     &#10;    1.3 - $(elementArray) 
        ///     &#10;    1.4 - $(object) 
        ///     &#10;    1.5 - $(jQuery object) 
        ///     &#10;    1.6 - $()
        ///     &#10;2: Creates DOM elements on the fly from the provided string of raw HTML.
        ///     &#10;    2.1 - $(html, ownerDocument) 
        ///     &#10;    2.2 - $(html, attributes)
        ///     &#10;3: Binds a function to be executed when the DOM has finished loading.
        ///     &#10;    3.1 - $(callback)
        /// </summary>
        /// <param name="selector" type="String">
        ///     A string containing a selector expression
        /// </param>
        /// <param name="context" type="">
        ///     A DOM Element, Document, or jQuery to use as context
        /// </param>
        /// <returns type="jQuery" />

        // The jQuery object is actually just the init constructor 'enhanced'
        return new jQuery.fn.init(selector, context, rootjQuery);
    };

Specifically, will the lines between the XML tags fail this rule? I guess if they have at least 1 space they won't which is good.

@zxqfox

This comment has been minimized.

Copy link
Member Author

zxqfox commented Aug 27, 2014

Oh right, this solution won't work. I think it's possible to add some special value like 'allowSlash' or something like that. It will pass any lines with triple slashed comments. Need an opinion of somebody else.

@zxqfox

This comment has been minimized.

Copy link
Member Author

zxqfox commented Aug 27, 2014

Or maybe just allow to pass any additional characters to this option to ignore after // and before first space?

@zxqfox zxqfox force-pushed the zxqfox:hotfix/require-space-after-comment-line-triple-slashed branch from 6d6042f to e5de62b Sep 5, 2014
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 5, 2014

Coverage Status

Coverage increased (+0.02%) when pulling e5de62b on zxqfox:hotfix/require-space-after-comment-line-triple-slashed into 7bd0438 on jscs-dev:master.

@zxqfox zxqfox force-pushed the zxqfox:hotfix/require-space-after-comment-line-triple-slashed branch from e5de62b to 29bb71b Sep 5, 2014
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 5, 2014

Coverage Status

Coverage increased (+0.02%) when pulling 29bb71b on zxqfox:hotfix/require-space-after-comment-line-triple-slashed into 7bd0438 on jscs-dev:master.

@@ -6,9 +6,12 @@ module.exports.prototype = {

configure: function(requireSpaceAfterLineComment) {
assert(
requireSpaceAfterLineComment === true,
requireSpaceAfterLineComment === true ||
requireSpaceAfterLineComment === 'allowSlash',
'requireSpaceAfterLineComment option requires the value true'

This comment has been minimized.

Copy link
@mikesherov

mikesherov Sep 20, 2014

Contributor

Please update the assertion message to reflect the new value.

@mikesherov

This comment has been minimized.

Copy link
Contributor

mikesherov commented Sep 20, 2014

Other than the one comment I mentioned, this looks good. @zxqfox can you make the changes and rebase so I can land this?

@mikesherov

This comment has been minimized.

Copy link
Contributor

mikesherov commented Sep 23, 2014

I'll fix this last piece myself, and land this now.

@mikesherov

This comment has been minimized.

Copy link
Contributor

mikesherov commented Sep 23, 2014

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.