moodlehq / moodle-cs

Moodle Coding Style
https://github.com/moodlehq/moodle-cs
GNU General Public License v3.0
18 stars 15 forks source link

Lang string sorted please #111

Closed stronk7 closed 6 months ago

stronk7 commented 7 months ago

Implement a new moodle.Files.LangFilesOrdering sniff in charge of check and fix string keys out of place.

Also, this moves the MoodleUtil tests to the new "fixtures" organisation that has been recently agreed (each test having its fixtures in same dir instead of global fixtures dir).

TODO:

This fixes #8

Created as draft, so we can start playing with it while fixing the points above.

Ciao :-)

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.46%. Comparing base (3966e9f) to head (238d3f4).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #111 +/- ## ============================================ + Coverage 97.29% 97.46% +0.16% - Complexity 656 706 +50 ============================================ Files 29 30 +1 Lines 1959 2090 +131 ============================================ + Hits 1906 2037 +131 Misses 53 53 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

marinaglancy commented 7 months ago

Hi! Is this a check specific to the Moodle core only or available for the plugins as well? Because plugins may not have "Deprecated since Moodle..." they will have their own version, i.e. "Deprecated since 1.2.1" or something like this

stronk7 commented 7 months ago

Right now, any comment not matching "Deprecated since Moodle" does stop the fixer to work for any problem detected after it. But the checker works ok and will report keys out of order.

Not sure if changing the restriction to be just "Deprecated since" is a good idea... or it is...

stronk7 commented 6 months ago

Ok, I think this is now ready. The 2nd commit has all the important stuff. Take a look to the details there:

This sniff examines exclusively lang files, only for Moodle 4.4 and up,
and has the following features:
 - Detect "groups" of lang strings, (normal, deprecated) within the
   lang file, checking for correct order within each group.
 - If other comments, different from the deprecated ones, are found,
   for safety the sniff will stop fixing strings after that point,
   although it will continue reporting them as out of order.
 - Detect wrong variable names (different from `$string`).
 - Detect duplicate strings within each lang file group.
 - Detect strings with incorrect trailing contents after it (php code,
   invalid comment...).
 - Detect strings out of order within each lang file group and fix them.

Notes about the fixing:
 - Due to CodeSniffer fixing limits, this has been the really complex
   thing to implement and to guarantee that the fixer works also for
   "big" lang files (say error.php and similar ones).
 - To do so, instead of fixing every case 1 by 1, while we are
   checking the file... we have to accumulate all the changes
   in custom structure and then, apply everything together as
   a big changeset.
 - As said above, for safety, if an unexpected comment is found,
   the fixer stops applying any change after that line. In order to
   allow it to continue fixing, the comment must be removed. Only
   comments allowed are the "^// Deprecated since " ones, used to
   detect the groups of strings.

The trickiest part has been to implement the fixer, because, by default, CodeSniffer fixer limits were not allowing us to make the changes in a performant way, but in loops (controlled by phpcbf itself and limited to a max of 50 (not enough for our big files). That restriction has been finally avoided by accumulating all the changes in a unique big changeset and applying everything together.

I've tested it against some big lang files (moodle.php, error.php, ...) and it seems to be working ok.

stronk7 commented 6 months ago

For the records, I've run it against the whole {{lang/en}} directory and it's processed in < 3 seconds, fixing all files but 3 (that have incorrect comments in the middle of the strings).

PHPCBF RESULT SUMMARY
-------------------------------------------------------------------------------
FILE                                                           FIXED  REMAINING
-------------------------------------------------------------------------------
/Users/stronk7/git_moodle/moodle/lang/en/edufields.php         1      0
/Users/stronk7/git_moodle/moodle/lang/en/access.php            1      0
/Users/stronk7/git_moodle/moodle/lang/en/filters.php           1      0
/Users/stronk7/git_moodle/moodle/lang/en/mimetypes.php         1      0
/Users/stronk7/git_moodle/moodle/lang/en/auth.php              14     0
/Users/stronk7/git_moodle/moodle/lang/en/competency.php        1      0
/Users/stronk7/git_moodle/moodle/lang/en/adminpresets.php      1      0
/Users/stronk7/git_moodle/moodle/lang/en/hub.php               FAILED TO FIX
/Users/stronk7/git_moodle/moodle/lang/en/install.php           2      0
/Users/stronk7/git_moodle/moodle/lang/en/group.php             12     1
/Users/stronk7/git_moodle/moodle/lang/en/currencies.php        1      0
/Users/stronk7/git_moodle/moodle/lang/en/pagetype.php          4      0
/Users/stronk7/git_moodle/moodle/lang/en/plagiarism.php        2      0
/Users/stronk7/git_moodle/moodle/lang/en/enrol.php             16     0
/Users/stronk7/git_moodle/moodle/lang/en/question.php          24     5
/Users/stronk7/git_moodle/moodle/lang/en/grades.php            FAILED TO FIX
/Users/stronk7/git_moodle/moodle/lang/en/availability.php      8      0
/Users/stronk7/git_moodle/moodle/lang/en/cache.php             32     0
/Users/stronk7/git_moodle/moodle/lang/en/mathslib.php          1      0
/Users/stronk7/git_moodle/moodle/lang/en/files.php             3      0
/Users/stronk7/git_moodle/moodle/lang/en/webservice.php        6      0
/Users/stronk7/git_moodle/moodle/lang/en/tag.php               16     0
/Users/stronk7/git_moodle/moodle/lang/en/plugin.php            13     0
/Users/stronk7/git_moodle/moodle/lang/en/grading.php           6      0
/Users/stronk7/git_moodle/moodle/lang/en/dbtransfer.php        2      0
/Users/stronk7/git_moodle/moodle/lang/en/langconfig.php        1      5
/Users/stronk7/git_moodle/moodle/lang/en/cohort.php            1      0
/Users/stronk7/git_moodle/moodle/lang/en/user.php              11     0
/Users/stronk7/git_moodle/moodle/lang/en/role.php              41     0
/Users/stronk7/git_moodle/moodle/lang/en/mnet.php              9      0
/Users/stronk7/git_moodle/moodle/lang/en/userkey.php           5      0
/Users/stronk7/git_moodle/moodle/lang/en/debug.php             4      0
/Users/stronk7/git_moodle/moodle/lang/en/editor.php            2      0
/Users/stronk7/git_moodle/moodle/lang/en/contentbank.php       7      0
/Users/stronk7/git_moodle/moodle/lang/en/backup.php            66     0
/Users/stronk7/git_moodle/moodle/lang/en/search.php            10     0
/Users/stronk7/git_moodle/moodle/lang/en/notes.php             5      0
/Users/stronk7/git_moodle/moodle/lang/en/payment.php           4      0
/Users/stronk7/git_moodle/moodle/lang/en/iso6392.php           17     0
/Users/stronk7/git_moodle/moodle/lang/en/timezones.php         1      1
/Users/stronk7/git_moodle/moodle/lang/en/privacy.php           6      0
/Users/stronk7/git_moodle/moodle/lang/en/moodle.php            FAILED TO FIX
/Users/stronk7/git_moodle/moodle/lang/en/block.php             3      0
/Users/stronk7/git_moodle/moodle/lang/en/xapi.php              1      0
/Users/stronk7/git_moodle/moodle/lang/en/license.php           1      6
/Users/stronk7/git_moodle/moodle/lang/en/error.php             1      0
/Users/stronk7/git_moodle/moodle/lang/en/portfolio.php         3      0
/Users/stronk7/git_moodle/moodle/lang/en/pp.php                1      8
/Users/stronk7/git_moodle/moodle/lang/en/course.php            6      0
/Users/stronk7/git_moodle/moodle/lang/en/blog.php              15     0
/Users/stronk7/git_moodle/moodle/lang/en/communication.php     2      0
/Users/stronk7/git_moodle/moodle/lang/en/courseformat.php      8      0
/Users/stronk7/git_moodle/moodle/lang/en/comment.php           1      0
/Users/stronk7/git_moodle/moodle/lang/en/calendar.php          30     0
/Users/stronk7/git_moodle/moodle/lang/en/analytics.php         28     0
/Users/stronk7/git_moodle/moodle/lang/en/h5p.php               21     0
/Users/stronk7/git_moodle/moodle/lang/en/customfield.php       1      0
/Users/stronk7/git_moodle/moodle/lang/en/bulkusers.php         1      0
/Users/stronk7/git_moodle/moodle/lang/en/reportbuilder.php     3      0
/Users/stronk7/git_moodle/moodle/lang/en/completion.php        16     0
/Users/stronk7/git_moodle/moodle/lang/en/form.php              2      0
/Users/stronk7/git_moodle/moodle/lang/en/my.php                6      0
/Users/stronk7/git_moodle/moodle/lang/en/admin.php             109    0
/Users/stronk7/git_moodle/moodle/lang/en/fileconverter.php     1      0
/Users/stronk7/git_moodle/moodle/lang/en/media.php             2      0
/Users/stronk7/git_moodle/moodle/lang/en/antivirus.php         10     0
/Users/stronk7/git_moodle/moodle/lang/en/imscc.php             3      0
/Users/stronk7/git_moodle/moodle/lang/en/table.php             1      0
/Users/stronk7/git_moodle/moodle/lang/en/rating.php            5      0
/Users/stronk7/git_moodle/moodle/lang/en/message.php           33     1
/Users/stronk7/git_moodle/moodle/lang/en/countries.php         1      0
/Users/stronk7/git_moodle/moodle/lang/en/badges.php            51     1
/Users/stronk7/git_moodle/moodle/lang/en/repository.php        1      24
-------------------------------------------------------------------------------
A TOTAL OF 742 ERRORS WERE FIXED IN 73 FILES
-------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 3 FILES
-------------------------------------------------------------------------------

Time: 2.91 secs; Memory: 42MB
stronk7 commented 6 months ago

Converted back to draft, while reviewing the results against lang/en I've detected a couple of border-cases that, while not the end of the world, better if we get them covered...

Thanks @andrewnicols!

stronk7 commented 6 months ago

Ok, back to ready. Now there are only 2 files that cannot be fixed (moodle.php and grades.php) completely, and that's because, by coincidence, the very same line where they have an "illegal" (unexpected) comment, is also out of order.

When that happens (for example, it doesn't happen in hub.php, previously failing), the only solution is to present the 2 problems and let the user manually decide (or remove unexpected comment or move the string+comment to a correct place).

I'm creating a script to verify that we are not losing or modifying any string with the fixes, but I really think that we aren't, heh.

Ciao :-)

stronk7 commented 6 months ago

And, also for the records, checking the whole Moodle codebase, in < 5 minutes, got:

----------------------------------------------------------------------------------------------------------------------------------------
A TOTAL OF 2417 ERRORS WERE FIXED IN 341 FILES
----------------------------------------------------------------------------------------------------------------------------------------
PHPCBF FAILED TO FIX 2 FILES
----------------------------------------------------------------------------------------------------------------------------------------

Time: 4 mins, 25.94 secs; Memory: 278.01MB

(the 2 failed files are the ones commented above (lang/en/grades.phpand lang/en/moodle.php, having both unexpected comment and wrong order in the same line).