Lang string sorted please #111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
|
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 |
|
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... |
Merged
stronk7
force-pushed
the
lang_string_sorted_please
branch
2 times, most recently
from
1f4df37 to
cfcf146
Compare
stronk7
force-pushed
the
lang_string_sorted_please
branch
10 times, most recently
from
e0b5ca4 to
3852b4b
Compare
|
Ok, I think this is now ready. The 2nd commit has all the important stuff. Take a look to the details there: 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 I've tested it against some big lang files (moodle.php, error.php, ...) and it seems to be working ok. |
stronk7
force-pushed
the
lang_string_sorted_please
branch
from
3852b4b to
24b8291
Compare
|
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). |
andrewnicols
requested changes
Mar 4, 2024
|
Converted back to draft, while reviewing the results against Thanks @andrewnicols! |
stronk7
force-pushed
the
lang_string_sorted_please
branch
3 times, most recently
from
be8a451 to
35ad0a6
Compare
|
Ok, back to ready. Now there are only 2 files that cannot be fixed ( When that happens (for example, it doesn't happen in 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
force-pushed
the
lang_string_sorted_please
branch
from
35ad0a6 to
bf12964
Compare
|
And, also for the records, checking the whole Moodle codebase, in < 5 minutes, got: (the 2 failed files are the ones commented above ( |
stronk7
force-pushed
the
lang_string_sorted_please
branch
from
bf12964 to
f25a5e8
Compare
We are going to use it with the LangFilesOrdering Sniff that needs to check when a given file is a lang one or no.
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.
stronk7
force-pushed
the
lang_string_sorted_please
branch
from
f25a5e8 to
238d3f4
Compare
andrewnicols
approved these changes
Mar 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement a new
moodle.Files.LangFilesOrderingsniff in charge of check and fix string keys out of place.^// Deprecated sincecomments and provides ordering within each of those "sections".$stringis allowed).Also, this moves the
MoodleUtiltests to the new "fixtures" organisation that has been recently agreed (each test having its fixtures in same dir instead of global fixtures dir).TODO:
lang/en/moodle.phpfor example.This fixes #8
Created as draft, so we can start playing with it while fixing the points above.
Ciao :-)