Description

thiemowmde

opened on Nov 13, 2020

Issue body actions

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?

Reacted by av1934413

Activity

1. edg2s commented on Jun 14, 2021

   

   edg2s

   on Jun 14, 2021

   Member

   More actions

   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).
2. thiemowmde commented on Jun 14, 2021

   

   thiemowmde

   on Jun 14, 2021

   Author

   More actions

   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?
3. edg2s commented on Jun 14, 2021

   

   edg2s

   on Jun 14, 2021

   Member

   More actions

   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.
4. thiemowmde commented on Jun 15, 2021

   

   thiemowmde

   on Jun 15, 2021

   Author

   More actions

   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?
5. Krinkle commented on Jul 27, 2021

   

   Krinkle

   on Jul 27, 2021

   Member

   More actions

   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.
6. thiemowmde commented on Aug 2, 2021

   

   thiemowmde

   on Aug 2, 2021

   Author

   More actions

   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-*
7. DannyS712 commented on Aug 4, 2021

   

   DannyS712

   on Aug 4, 2021

   Contributor

   More actions

   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.
8. Krinkle

   added

   Status: Declined

   Type: Problem

   on Aug 4, 2021
9. Krinkle

   closed this as completedon Aug 4, 2021
10. thiemowmde commented on Sep 2, 2021

    

    thiemowmde

    on Sep 2, 2021

    Author

    More actions

    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?
11. Krinkle commented on Sep 2, 2021

    

    Krinkle

    on Sep 2, 2021

    Member

    More actions

    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.
12. edg2s commented on Sep 2, 2021

    

    edg2s

    on Sep 2, 2021

    Member

    More actions

    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
13. wmfgerrit

    added 2 commits that reference this issue on Dec 3, 2022

    Fixing some JSDoc warnings

    835ee06

    Update git submodules

    28788a1

Issue actions