Part of meta-issue #1518116: [meta] Make Core pass Coder Review
Remaining tasks:
- (done) open issues to fix false negatives for coder listed below
- (skipping this) Decide on #47 (to break arrays over 80 char).
- keep rerolling as needed
We are not changing @param @return types or typehints in function signatures in this issue.
See #66 and #2288793: Add missing and fix some types in core docblocks and add some typehinting for locale module
Other issues:
Coder issues that helped reduce false errors that have been fixed while working on this issue
- #2288835: not ignoring doc blocks in functions completely, still error on /** @var */
- #2270551: Modify sniff to allow $ (this) in @return
- #2288837: Modify sniff to allow $ (this) in @return to not have a comment
- #2288851: relax indentation errors until chaining standard is decided
- #2279131: False positives for "Comment missing for @throws tag in function comment"
- #2288823: @defgroup and @addtogroup should not ERROR with Missing short description in doc comment
Things not fixed in this issue:
- Replacing * Implements ... with {@inheritdoc}
- .js files because #1778828: [policy, no patch] Update JS coding standards not done
- .css files
- rewording long comments to be (under) 80 chars (since that might change the meaning of the comments to be incorrect and makes it too difficult to review)
Review hints
To install code sniffer:
- Install composer (the install composer instructions that are part of the drush ones might be nice https://github.com/drush-ops/drush/blob/master/README.md )
- get phpcs (2.0 dev required to work with 8.x coder_sniffer), by for example:
composer global require squizlabs/PHP_CodeSniffer:2.0.x-dev
- get the 8.x version of coder (which has the Drupal sniffer rules), instructions on https://www.drupal.org/node/1419988 (which say to add
"minimum-stability": "dev"
to composer.json, and thencomposer global require drupal/coder:dev-8.x-2.x
(we used to have to use the fork for the Coder meta, but changes to Coder 8.x seem to not make that necessary right now) - add the composer vendor bin to your path so you can run phpcs
- (not currently needed) patch coder (to ignore type things, or to ignore rules that are broken)
Which should end up with something like this example this composer.json (or just use this and then run composer update)
{
"require": {
"squizlabs/PHP_CodeSniffer": "2.0.x-dev",
"drupal/coder": "dev-8.x-2.x"
},
"minimum-stability": "dev"
}
run like:
phpcs --extensions=php,module,inc,install,test,profile,theme --standard=$path_to_sniffer_Drupal/coder-8.x/coder_sniffer/Drupal --warning-severity=0 -- core/modules/locale
or link the drupal standards like:
ln -s /Users/me/.composer/vendor/drupal/coder/coder_sniffer/Drupal /Users/me/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal
and then run like:
phpcs --extensions=php,module,inc,install,test,profile,theme --standard=Drupal --warning-severity=0 -- core/modules/locale
which will ignore .js and css files
and not print out warnings (only prints errors)
Comment | File | Size | Author |
---|---|---|---|
#95 | interdiff.1533250.94.95.txt | 1.62 KB | YesCT |
#95 | drupal-make_locale_module_pass_coder_review-1533250-95.patch | 114.14 KB | YesCT |
#94 | drupal-make_locale_module_pass_coder_review-1533250-94.patch | 113.19 KB | YesCT |
#92 | interdiff.leftout.txt | 8.58 KB | YesCT |
#92 | drupal-make_locale_module_pass_coder_review-1533250-92.patch | 115.73 KB | YesCT |
Comments
Comment #1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedPostponed because of #1326618: Clean up API docs for locale module
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedThe locale module is moving much closer to being in good shape from the docs API perspective. Can someone please post here the output from a Coder review of the locale module?
I am unable to run Coder at the moment and would appreciate seeing what gets sniffed as I roll a patch for some code style issues I saw while working on the Locale docs review. Thanks in advance.
I am also removing Novice tag since Coder output still requires some more advance interpretation for possible positive code sniffs.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedIn test LocaleCompareTest.php, there is a member call called $translation_directory. This should be converted to a camel case variable like $translationDirectory.
In LocaleExportTest.php, there is a member called $admin_user. This too should be changed to a camel case variable. Same case with LocaleImportFunctionalTest.php.
Comment #4
TravisCarden CreditAttribution: TravisCarden commentedPostponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!
Comment #5
TravisCarden CreditAttribution: TravisCarden commentedComment #6
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commentedI'm starting on coder review
Comment #7
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commentedStarted working on the coder review
Comment #8
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commentedUploading patch.
Comment #10
sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commentedComment #11
fran seva CreditAttribution: fran seva commentedI'm going to check whats happening with the patch and the code review.
I assigned myself the issue
Comment #12
fran seva CreditAttribution: fran seva commentedThe phpcs founds 49 ERROR(S) AND 4 WARNING(S) AFFECTING 36 LINE(S)
Attach the file with the output.
Comment #13
fran seva CreditAttribution: fran seva commentedI've attached the file with the phpcs output apply to locale module.
The file in #12 is wrong just have one file review.
The command run to get the coder reviewer was:
~/.composer/vendor/bin/phpcs --extensions=php,module,inc,install,test,profile,theme --standard=/Users/fran/.composer/vendor/drupal/coder/coder_sniffer/Drupal drupal/core/modules/locale/
@TravisCarden I see that the output check as an error the proposal change in the comment #3.
In comment #3 it's said to change non camel variable to camel but the output said the oposite. Am I using the wrong phpcs?
[:edit]
[1] https://github.com/TravisCarden/coder/tree/issue-1518116
Comment #14
TravisCarden CreditAttribution: TravisCarden commentedHey, @fran seva. I can see how this could be confusing. We use camelCase on object properties but underscores in local variables, so there are times for each pattern. Sniffer is pretty good about telling you when to use which. Thanks!
Comment #15
fran seva CreditAttribution: fran seva commented@TravisCarden for the confirmation!! I'm going to check the las patch submitted comparing its changes with the sniffer output.
Comment #16
fran seva CreditAttribution: fran seva commentedHi all!!
I've been working in the review and:
* I've tried to apply the attached patch to start working with the patch applied but has been impossible...I tried to apply a reroll but after read the dedicated page to explain reroll understood I have to start from a valid patch and there is no one.
* The currently state is attached as a file. @TravisCarden I have an strange case related with LocateLookupTest.php. The Errors are really strange. If you apply the changes the result is a rare indented code. Could you confirm that it's correct?
As you can see in the attached file, the mayor errors are related to indentation and comment block (needs more coments)
FILE: ...-dev/core/modules/locale/tests/Drupal/locale/Tests/LocaleLookupTest.php
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
177 | ERROR | Line indented incorrectly; expected 35 spaces, found 27
178 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
179 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
181 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
182 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
184 | ERROR | Line indented incorrectly; expected 37 spaces, found 10
185 | ERROR | Line indented incorrectly; expected 37 spaces, found 19
187 | ERROR | Closing brace indented incorrectly; expected 27 spaces, found 8
188 | ERROR | Closing brace indented incorrectly; expected 4 spaces, found 6
--------------------------------------------------------------------------------
Comment #17
fran seva CreditAttribution: fran seva commented@TravisCarden forget what I said about the indented code I solved it.
Now just remain 29 errors, all of them "Missing function doc comment"
Comment #18
fran seva CreditAttribution: fran seva commentedpatch after apply all changes to pass phpcs validations.
Comment #19
drupalninja99 CreditAttribution: drupalninja99 commentedComment #21
fran seva CreditAttribution: fran seva commentedThe test that are failing are:
* LocalUpdateTest.php
* LocalUpdateInterfaceTest.php
* LocalUpdateCronTest.php
I have run them in my local environment and there are php errors...I'm going to check them.
Comment #22
fran seva CreditAttribution: fran seva commentedI'm still working on it
Comment #23
fran seva CreditAttribution: fran seva commentedAfter review the test for LocalUpdateCronTest I think the errors could be caused by a bad configuration in locale module in my environment.
I'm going to check all the errors but I have some questions:
* Are there any section in Drupal.org with information about how configure the locale module to run all test correctly?
* The errors in the test machine for patch #18 for LocalUpdateCronTest are the same, so could be possible that the locale test for cron had an error in the initialization? I'm confuse, because if I don't apply the patch #18 the tests don't show any error
I'm thinking on it but if someone know anything about this questions will be great!!
Comment #24
tstoecklerGot as far as LocaleStringTest:
This should be broken after "same", not after "the".
This should be reverted. The current code is fine as is.
This should be something like "The typed config manager." (don't forget the trailing period.)
This should be removed. It can't be seen from the patch context, but both of these are @param.
All of these should be just
It seems you're introducing a double space here?!
Our standard is that the one-line description does not exceed 80 characters, so we'll need to shorten that.
We can just remove the "Helper function that" part and start directly with "Returns..."
Should be "Test*s*".
Comment #25
fran seva CreditAttribution: fran seva commentedI'm rerolling...
Comment #26
fran seva CreditAttribution: fran seva commented@tstoeckler I have applied the changes and after review the code the followings error are showed by phpcs:
In LocaleLookupTest.php, after revert the changes the current phpcs for Drupal return bad indentation.
In TranslateFormBase.php there is an error that I don't know how fix it (Should be easy but...phpcs is telling me that I'm a looser :( ) The code are in [1]
Also, I have made the changes to fix the error with the method locale_translation_batch_status_check
[1]
Comment #28
fran seva CreditAttribution: fran seva commentedFix the patch...
Comment #29
fran seva CreditAttribution: fran seva commentedComment #30
tstoeckler@franSeva Thanks a lot for your hard work on this. I found a few more things that need fixing, sadly. Can you give this another shot?
These changes are incorrect. Please revert them.
This should be just {@inheritdoc}.
Our standard dictates that the one-line description must not exceed 80 characters, so this needs to be shortened in some way. I would suggest
Agian, this should be shortened. Maybe just
The parentheses should be kept here.
There should be a newline before this line and the "//" should be removed.
Again, this needs to be shortened. Perhaps: "Concstructs batch operations..."
Suggestion: "Parses a JavaScript file for translatable string and saves them to the database."
I think "Re-creates" should have a lowercase "c".
This should be just "{@inheritdoc}"
This should be "{@inheritdoc}"
Comment #31
fran seva CreditAttribution: fran seva commented@tstoeckler sure. I will change and review it.
Thanks for review!!!
Comment #32
fran seva CreditAttribution: fran seva commentedThe patch #28 doesn't apply, needs reroll.
I'll do it.
Comment #33
fran seva CreditAttribution: fran seva commentedRe-roll patch:
- Locale.module has been modified. Hook_help has change and now use route_name
- locale.bulk.inc has remove code
attach the patch
Comment #34
fran seva CreditAttribution: fran seva commentedComment #35
fran seva CreditAttribution: fran seva commented@tstoeckler I have made all changes but phpcs still shows errors in point 1.
phpcs: @return data type must not contain "$"
phpcs: Missing comment for @return statement
May be that my phpcs is outdate, I'm going to check it.
Attach the patch and the interdiff!! XD
Comment #36
YesCT CreditAttribution: YesCT commentedhttps://drupal.org/node/1354#types
says that $this can be a type,
so I guess we need to update phpcs rules...
the meta #1518116: [meta] Make Core pass Coder Review has a "blockers" section.
maybe we need something in the coder project issue queue like #1805544: Modify sniff for "Implements hook_foo_BAR_ID_bar() for xyz_bar()", but for updating the rule to allow $this in @return?
Comment #37
fran seva CreditAttribution: fran seva commented@yesct I have checked the last coder module and currently don't allow "$" in @return comments. So, as you said, would be needed an update in phpcs rules.
Comment #38
tstoecklerAgain, great work @fran seva!
Sadly I couldn't yet mark this RTBC, but there are two super small errors in the patch:
Seems like this removes the blank line between the two methods. That should be kept.
Here also the blank line should be kept.
It would be great if you could fix that, then we can set this RTBC :-)
Comment #39
fran seva CreditAttribution: fran seva commented@tstoeckler I didn't seen the llines in the interdiff :(
I'm on it :)
Again, thanks for review!!!
Comment #40
fran seva CreditAttribution: fran seva commentedhere is the patch and its interdiff :)
Comment #41
tstoecklerYay. fran Seva, you rock!
Thanks for your persistence.
Comment #42
YesCT CreditAttribution: YesCT commentedadding this blank line before the end of the case } seemed weird, so I checked https://drupal.org/coding-standards#controlstruct
and also installed and ran the sniffer according to the summary in #1518116: [meta] Make Core pass Coder Review
still passes without the blank line before the switch closing }.
this looks like it wrapped too soon.
https://drupal.org/coding-standards/docs#drupal
"lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over..."
so re-wrapped this.
still passes coder review.
maybe we could look and see if coder has an issue to check that "as close to 80" rule.
I might look at this some more later.
Comment #43
YesCT CreditAttribution: YesCT commented@fran seva can you open an Coder issue for #37 and link it here in the issue summary?
(maybe also for #42 2.)
Comment #44
fran seva CreditAttribution: fran seva commented@YesCT thanks for review and the patch.
About #43 I'm on it
Comment #45
tstoeckler...I'm a bit annoyed at @YesCT having to show off that she can be even more nitpicky than me, but the patch looks good!
(...I'm kidding of course :-))
Comment #46
fran seva CreditAttribution: fran seva commentedI have created the issue [1] but I'm not sure about its category and version :S
Could anyone review it?
[1] https://drupal.org/node/2270551
Comment #47
YesCT CreditAttribution: YesCT commentedAre we not fixing errors like:
FILE: ...rupal2/core/modules/locale/lib/Drupal/locale/Form/TranslateEditForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
184 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
?
Comment #48
webchickComment #49
YesCT CreditAttribution: YesCT commented1.
Reviewing the patch a bit more into it,
found:
oops a double space.
fixed that.
2. So I ran
/Users/ctheys/.composer/vendor/bin/phpcs --standard=/Users/ctheys/.composer/vendor/drupal/coder/coder_sniffer/Drupal core/modules/locale
And found more things:
Patch fixes many more
2.a.
A comma should follow the last multiline array item.
2.b.
WARNING | Line exceeds 80 characters; contains ...
2.c.
Case breaking statements must be followed by a single blank
| | line
I checked and these were fixed in other places in the patch already, so I could not see why not to fix these.
========
Updated the issue summary with what I think a committer would want to know about what is fixed and what isn't.
--
Next:
Missing parameter type at position ...
Comment #50
YesCT CreditAttribution: YesCT commentedoh, a couple of little fixes
* @return array $filter_values
was interesting
cause the error was
124 | ERROR | Return comment must be on the next line
and I was like: it is!
But there is a var name. :) took that off.
Comment #51
YesCT CreditAttribution: YesCT commentedNow
Missing parameter type at position
Comment #52
YesCT CreditAttribution: YesCT commentedcorrecting the indent here means we have to fix the line below also.
it is also
@param
not
@params
------
that ends reading through the whole patch and running coder on it.
ok. I'm done for a bit. :)
Now we just decide on #47 I guess.
Comment #53
tstoecklerWe don't enforce #47 very strictly throughout core and I always under the impression that that's merely a suggestion not a strict rule.
Interdiffs look good.
Comment #54
YesCT CreditAttribution: YesCT commentedcreated (novice) #2271485: Move LocaleStringTest $only_translated = array('untranslated' => FALSE); out of accidental comment
Comment #55
fran seva CreditAttribution: fran seva commented@tstoeckler in comment #53 you mean that in the Travis Garden's fork branch issue-1518116 those kind of errors are not being checked?
I ask because when I ran the phpcs the error reported by @YesCT didn't appear and I wanna be sure that I'm using the correct coder version.
@YesCT thanks for your review and the corrections in the issue #2270551: Modify sniff to allow $ (this) in @return
Comment #56
tstoecklerRe @fran seva: I didn't run phpcs at all, I just reviewed the patch manually, so I can't really say anything about that.
Comment #57
fran seva CreditAttribution: fran seva commentedComment #58
xjmReroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.
Comment #60
fran seva CreditAttribution: fran seva commented@xjm I have tried the PSR-4 reroll :)
I don't attach the interdiff. Is necessary in a reroll?
Comment #61
YesCT CreditAttribution: YesCT commented@fran seva interdiffs are tricky on rerolls. Sometimes I copy and paste the conflicts and how I resolve them. But the only way I really know how to review them is to do my own reroll and them compare. ;)
-----
I resolved the conflicts differently.
mine are on the left (<)
1.
I wrapped this so that (other than... was on the line
2.
and
I did this which was in the patch before, of adding the space before the .
which seems to be missing from #60
3.
and I'm not sure why patch in #60 deleted SystemLocalTasksTest
but my patch leaves this in.
---
no interdiff since #60 didn't apply already.
Comment #62
fran seva CreditAttribution: fran seva commented@YesCT thanks for your tip on reroll and how review it.
About:
1. OK
2. I didn't see the space. I remember this was a conflict in the reroll and I check the text and the Drupal::url but not the space
3. Following the steps in [#424758] this file was check to be deleted so I create the patch with git -C -M to insert the deletion.
I did it because I read this in the reroll psr4 steps:
@YesCT thanks again for your review. I'm going to try to make the patch again to check point 3. and understand what I did wrong.
Comment #63
YesCT CreditAttribution: YesCT commentedhm.
re 3.
running php core/scripts/switch-psr4.sh on 8.x head deletes a file.
core/modules/system/tests/Drupal/system/Tests/Menu/SystemLocalTasksTest.php
Since that is a problem in head, we shouldn't be messing with it here.
Made a separate issue #2286241: Move stray SystemLocalTasksTest to PSR-4 location
Comment #64
fran seva CreditAttribution: fran seva commented@YesCT I'll wait till this #2286241: Move stray SystemLocalTasksTest to PSR-4 location will be fixed.
Comment #65
tstoecklerThanks for the explicit merge-interdiff (or however to call that). That is helpful. Looks great.
Comment #66
TravisCarden CreditAttribution: TravisCarden commentedSorry to come late to the game with this feedback, but type hinting doxygen directives (
@param
and@return
) has been excluded from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* out of respect for committers who feel that those changes make these patches painful for them to review. If we can just remove those from this patch, the committers will appreciate it, and we'll be more likely to get this committed on the first try. Note: If you're following the instructions on the meta issue for using our Coder spork, then Sniffer won't complain about missing type hinting.Comment #67
YesCT CreditAttribution: YesCT commentedUsing the 8.x branch of coder eliminates some false positive errors.
Not the github fork.
Maybe we need a fork of the 8.x stuff? or directions for how to silence the type hinting stuff?
I'm ok with taking it out, and making a separate issue... if that is what committers want.
Comment #68
YesCT CreditAttribution: YesCT commentedOk. I'll split this into two issues.
Comment #69
YesCT CreditAttribution: YesCT commentedI have to double check if I did all the diffs correctly.
And need to open another issue for the @param @return and typehint changes.
And need to provide instructions for reviewing (might want to combine the fork which ignores the types, and the 8.x branch of code sniffer which has eliminates 8.x false positive errors).
But here are the files for now. I'll be back at this in just a bit.
Comment #70
YesCT CreditAttribution: YesCT commentedupdated issue summary with more separate coder sniffer issues,
next, a new patch (and interdiff) with more fixes found using the 8.x
Comment #71
YesCT CreditAttribution: YesCT commenteddone for now. let's get some feedback as to if this is reviewable. (see review hints in the issue summary)
interdiff is those things I found while running 8.x coder_sniffer on them.
Comment #72
tstoecklerThe current code is just wrong, let's please not re-introduce that code just because of issue queue madness.
I think we generally try to avoid "don't" style abbreviations. Also I think the sentence-flow could be improved. Not a native speaker, but shouldn't be "for overriding"? Also the constants use "overwrite" instead of "override", that looks very strange. Not sure which one is correct, though.
As far as I know \stdClass is actually preferred over object, but I might be mistaken.
Comment #73
YesCT CreditAttribution: YesCT commentedI promise to do the issue (#2288793: Add missing and fix some types in core docblocks and add some typehinting for locale module) that actually changes @param and @return and type hinting. But for this coding standards one, I'd rather get this in, and @jhodgdon in irc confirmed @TravisCarden 's #66 request.
I'll fix 1.
And 2. https://www.drupal.org/node/1354#types
object (NOT "stdClass")
-
working on this now.
Comment #74
YesCT CreditAttribution: YesCT commentedjust a reroll.
3 conflicts, but they were simple. In 2, just redid the patch changes on the new head lines, and in 1 it was just a context line that changed in head, and kept head. :)
Comment #75
YesCT CreditAttribution: YesCT commentedhere is that re-wording.
next, re-run the sniffer on this to see if this is covering all it should.
nothing really novice at this point. :)
Comment #76
YesCT CreditAttribution: YesCT commentedI reran the sniffer, and all the errors it reports are due to the sniff issues (see the list of related issues) or are things we are not going to fix in this issue.
I think this is ready again.
Comment #77
tstoecklerI'm very, very frustrated with the process of this issue, but the patch itself is RTBC.
Thanks for your tireless effort @YesCT and @fran seva!!!
Comment #78
TravisCarden CreditAttribution: TravisCarden commented@YesCT requested my eyes on this patch. Everyone's done a great job! There are just a few little things left. I've gone ahead and fixed them myself. (Attached.) It just all stuff.
@YesCT, are you using the Coder 8.x branch? (I notice some of the related issues filed against Coder are against it.) I haven't had the time to get it working, so I'm still using my spork as described on the meta. If we've figured out how to use 8.x, it would be good to update the instructions there. Since there remains a question, I'm attaching a bash history of my entire review process. (See issue-1533250-review-history.txt attached.)
Comment #79
tstoecklerThe additional fixes look good, thanks. Sadly, this is no longer RTBC:
Please revert this. This is being handled in #2271485: Move LocaleStringTest $only_translated = array('untranslated' => FALSE); out of accidental comment.
Since we're changing this anyway (now), let's lowercase 'Another' and 'Language'.
Comment #80
TravisCarden CreditAttribution: TravisCarden commentedThank you, @tstoeckler. Here you go.
Comment #81
tstoecklerThanks for the quick turnaround. Let's see if my 6th RTBC will be the last...
Comment #82
jhodgdonI hate these patches... I feel obligated to review them carefully before I commit them. So of course I find an error or two and they are so huge, you don't want to make them go through yet another cycle of patch/review.
Still, in locale.module:
I'm assuming the previous documentation here was saying that it either creates or re-creates the file, right? The new docs change the meaning. Did someone check to see which was correct? If not I would just leave this as is.
I'm also not sure about all the explicit adds of "public" to methods on various classes. Difficult for me to evaluate whether they are correct or not.
So... at that point in the patch (I got about 1/3 of the way through), I gave up... sorry.
Comment #83
kim.pepperFrom the PHP docs: http://www.php.net/manual/en/language.oop5.visibility.php
Comment #85
YesCT CreditAttribution: YesCT commentedComment #86
kim.pepperI think we should be adding explicit
public
visibility to methods. I was pointing out that we are safe to do so, where no visibility modifier is provided.Comment #87
YesCT CreditAttribution: YesCT commentedoops. #84 was messed up.
Here is what it should have been.
@jhodgdon thanks for getting that far. I checked and (Re-) is correct, so I put that back in.
Comment #88
YesCT CreditAttribution: YesCT commentedreroll. auto 3 way merge. no conflicts.
Comment #89
YesCT CreditAttribution: YesCT commented1.
this was strange to start with.
usages:
So how about:
2.
also added a period to a @todo sentence.
---
read through the whole thing again, still looks good to me.
Comment #90
YesCT CreditAttribution: YesCT commentedreroll for #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
Some things to check about the moving of the getInfo descriptions to the one line summary on the classes.
patch coming.
Comment #91
YesCT CreditAttribution: YesCT commentedtried to reword the one line summary for 80 chars.
I'm currently using
in composer.json
{
"require": {
"drush/drush": "dev-master",
"squizlabs/PHP_CodeSniffer": "2.0.x-dev",
"drupal/coder": "dev-8.x-2.x"
},
"minimum-stability": "dev"
}
run composer update from ~/.composer to get the latest 8.x fixes to coder
linked the standard
ln -s /Users/me/.composer/vendor/drupal/coder/coder_sniffer/Drupal /Users/me/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal
and run like
phpcs -- --standard=Drupal --extensions=php,module,inc,install,test,profile,theme core/modules/locale
Comment #92
YesCT CreditAttribution: YesCT commentedok. here is 91, but simplified, to take out any comment changes or anything that might make it more involved to review (like rewording comments).
If someone reviews this, please specify if you are reviewing 91 or 92
Attaching what I left out of 92.
Comment #93
herom CreditAttribution: herom commentedreviewed patch #92.
one relevant error from coder:
two cases with multiple-spacing in the lines. should they be fixed here?
Comment #94
YesCT CreditAttribution: YesCT commentedreroll for
#1978926: Convert locale_translation_status_form to a Controller
#2300829: Clean up LocaleConfigManager / TypedConfigManager features and cache duplications
#1978926: Convert locale_translation_status_form to a Controller
will address feedback next.
Comment #95
YesCT CreditAttribution: YesCT commentedfixes from feedback in #93.
re-ran the sniffer, and looks like got the actual errors that we are going to address here
Comment #96
herom CreditAttribution: herom commentedI didn't find any more issues.
Comment #97
YesCT CreditAttribution: YesCT commentedthe scope of this is smaller than actually making it pass Coder. retitling.
several errors we were ignoring have had their rules updated in Coder. (Thanks!)
So taking those notes out of the issue summary, and just listing the related issues.
updated review/install instructions. I'm not sure why we dont need the fork that ignores the @param and @return rule.. but Coder 8.x seems to be ok without that fork (for this issue)
Comment #98
YesCT CreditAttribution: YesCT commentedhtml
Comment #99
YesCT CreditAttribution: YesCT commentedif we have to reroll this again. after the reroll, we could take out these two changes (since they require more review work). Leaving in for now.
Comment #100
alexpottCommitted 12c2fbb and pushed to 8.x. Thanks!