mediawiki/msg-doc doesn't accept valid docs with 1 entry

[thiemowmde]

The plugin blocks my patch with "All possible message keys should be documented". But I documented everything:

// The following messages are used here:
// * advancedsearch-sort-*
var msg = mw.message( 'advancedsearch-sort-' + name.replace( /_/g, '-' ) );

(from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AdvancedSearch/+/627230/6/modules/ui/ext.advancedSearch.SortPreference.js#15)

It appears like the plugin assumes a list with 1 entry must be wrong: https://github.com/wikimedia/eslint-plugin-mediawiki/blob/master/src/utils.js#L54. But this is the full list. There is nothing more to say at this point.

Adding something like * … just to silence the error doesn't work either: https://github.com/wikimedia/eslint-plugin-mediawiki/blob/master/src/utils.js#L13.

Can we please relax the plugin and make it accept a single list entry?

[edg2s]

I considered removing the restriction, but on reflection, if you can't list complete message keys then just add an inline ignore for the rule. The comment used in the example above isn't of much use as it doesn't make the message keys any more grep-able which is the purpose of this rule ("advancedsearch-sort-" as a string already exists, and "advancedsearch-sort-myname" is still not grep-able).

[thiemowmde]

Maybe that wasn't a good example. What if there really is only a single message? At least for the moment? How is this a violation?

[edg2s]

That would be a valid case, but as you say that is a temporary situation (and I think pretty rare). I would just use something like:

// The following messages are used here:
// * sort-name
// * TODO: other sort modes to be supported
$foo.addClass( 'sort-' + x );

or just use a disable directive (along with the single item list). Once the list expands to two items, eslint will detect that the disabled directive is not required.

[thiemowmde]

I don't know if it's "temporary". The sniff is useful. But what I don't get is why it needs to decide that 2 entries are valid, but 1 is not? What is the benefit of this additional constraint? How does it help? How often does this happen, and how often is it an error and not intentional as in my situation?

[Krinkle]

I think instinctively, it feels like needless complexity (indirection, abstraction) for something to appear variable when it truly isn't. This isn't always wrong for its own sake of course, but I don't recall having seen this it in a situation where I didn't find it to be part of a larger code smell.

Short of this happening at least once, it might be beneficial for now to have this rule be simpler, and I could even see it as useful to have that little tiny bit of resistence to think if maybe there's a different way to solve this, or if not, to simply inline disable it and possibly report back here.

[thiemowmde]

I found a solution I can live with: I listed all messages that currently exist, and a placeholder to highlight the fact that it's not limited to this list.

// The following messages are used here:
// * advancedsearch-sort-relevance
// * advancedsearch-sort-*

[DannyS712]

I found a solution I can live with: I listed all messages that currently exist, and a placeholder to highlight the fact that it's not limited to this list.

// The following messages are used here:
// * advancedsearch-sort-relevance
// * advancedsearch-sort-*

I think this is better than allowing just one entry in the list, because that might be because the developer forgot to list the others. I propose to decline this task in favor of using this solution instead.

[thiemowmde]

Here is another example where the sniff is just not helpful. Apparently I'm not even allowed to make my comment explain why it doesn't make sense to have a list. Not even when I obey the rule that I must list two things. What is this trying to protect me from?

[Krinkle]

Here is another example where the sniff is just not helpful. Apparently I'm not even allowed to make my comment explain why it doesn't make sense to have a list. Not even when I obey the rule that I must list two things. What is this trying to protect me from?

I believe it has already been explained what the rule protects you from.

ve.ui.MWIncludesContextItem.prototype.getLabelMessage = function () {
    var key = {
        'mw:Includes/NoInclude': 'visualeditor-includes-noinclude-start',
        'mw:Includes/NoInclude/End': 'visualeditor-includes-noinclude-end',
        'mw:Includes/OnlyInclude': 'visualeditor-includes-onlyinclude-start',
        'mw:Includes/OnlyInclude/End': 'visualeditor-includes-onlyinclude-end',
        'mw:Includes/IncludeOnly': 'visualeditor-includes-includeonly'
    }[ this.model.getAttribute( 'type' ) ];
    // The following messages are used here:
    // * visualeditor-includes-*
    // * (see above)
    return key ? mw.message( key ).text() : '';

I think the above example is unrelated to the example this issue was created for. This example is variable, and there are multiple messages. This is what eslint-disable-next-line is for. The warning is understood by the developer and the risk is either accepted, or addressed in another way already. I don't think it warrants an inline explanation.

[edg2s]

There is no need to document the message strings in this example as they are already written out in full in the lines above. The purpose of this rule is to try to encourage message strings to be written out in full near the code that uses them, so someone searching the code for a message string will find it.

In this case I would recommend the following comment above the return:

// Message keys already documented above
// eslint-disable-next-line mediawiki/msg-doc