Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1518116: [meta] Make Core pass Coder Review
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal-make_rdf_module_pass_coder_review-1533380-36.patch | 30.76 KB | Jalandhar |
#36 | rdf_coder_issues.txt | 8.91 KB | Jalandhar |
#30 | drupal-make_rdf_module_pass_coder_review-1533380-30.patch | 33.98 KB | Jalandhar |
#3 | 8.x...1533380.patch | 891 bytes | lotyrin |
Comments
Comment #1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedPostponed because of #1418980: Clean up API docs for rdf
Comment #2
lotyrin CreditAttribution: lotyrin commentedComment #3
lotyrin CreditAttribution: lotyrin commentedThis one was easy. No doc changes.
Comment #4
xjmLet's update the issue here with:
In each case note any failures that were not corrected, and why.
Comment #5
lotyrin CreditAttribution: lotyrin commentedUnassigning because I'm not sure I'll have time for these issues and I want to make sure people know they can jump in without stepping on my toes.
Comment #6
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 #7
TravisCarden CreditAttribution: TravisCarden commentedComment #8
Jalandhar CreditAttribution: Jalandhar commentedHi,
I have started working on it.
Comment #9
Jalandhar CreditAttribution: Jalandhar commentedHi,
I have done all coder errors related to rdf module except the below 1 error. Please suggest me on how can I do the below one.
FILE: ...ewdrupal8/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
30 | ERROR | Class property $base_uri should use lowerCamel naming without
| | underscores
--------------------------------------------------------------------------------
Attaching the patch for remaining all coder errors.
Comment #11
Jalandhar CreditAttribution: Jalandhar commentedAnother try at the patch.
Comment #12
scor CreditAttribution: scor commentedThat's fine by me, there is no reason to have it called $base_uri and not $baseUri, so feel free to make that change on line 30 and in the rest of that file.
Comment #14
scor CreditAttribution: scor commentedWe need to keep this extra CR here. See comment explaining why we have an extra CR here.
Renaming those to testSomething() will trigger the test suite to try to run those as indivual tests. They should be renamed to something like runTestSomething()
Comment #15
Jalandhar CreditAttribution: Jalandhar commentedHi Scor.
Thanks for your suggestions. I have made the changes that you have said in the comment #14.
Regarding extra CR, coder is returning error if I keep extra CR as
FILE: ...ww/drupal/core/modules/rdf/lib/Drupal/rdf/Tests/StandardProfileTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
476 | ERROR | Array indentation error, expected 6 spaces but found 0
--------------------------------------------------------------------------------
Done the remaining errors and attaching the patch.
Comment #17
dman CreditAttribution: dman commentedWould
meet the requirement?
Comment #18
Jalandhar CreditAttribution: Jalandhar commentedHi Scor,
The failed test cases in comment #15 is in image and view module. can you please suggest me on how I can fix it?
Comment #19
dman CreditAttribution: dman commented@jalandhar It could be that the testbot is just a bit sick right now. I've seen it happen a few times today, and the system is working really hard.
"The test did not complete due to a fatal error." appears to be something with the testing system, not the code.
https://drupal.org/project/issues/search?issue_tags=Random%20test%20failure
Generally it may be worth waiting a few hours then queuing for re-test.
Comment #20
dman CreditAttribution: dman commented15: drupal-make_rdf_module_pass_coder_review-1533380-15.patch queued for re-testing.
Comment #21
scor CreditAttribution: scor commentedI would expect to see a empty line above a docs block.
There is usually an empty line after the last method of a class, see https://drupal.org/node/608152#indenting
I find this much harder to read. Is that something coder review is complaining about? This is a commented piece of code pending a todo, not an actual comment.
ditto. and there are more further.
Comment #22
scor CreditAttribution: scor commented@dman re #17, good idea, maybe something like this:
Comment #23
dman CreditAttribution: dman commented3,4
Yes, coder review will complain about comments longer than 80 cols.
I'm cleaning up some of my own code tonight, and yeah, my temporarily commented out code blocks count as misformatted comments.
It also complains about indenting if we try to keep the old code layout.
And yes, it does become harder to read if we comply.
Either have to remove temporary cruft altogether, make an exception for those cases, or scrunch the code.
Comment #24
Sachini CreditAttribution: Sachini commentedI made some modifications to go with Object Oriented coding standards.
My patch contains changes by jalandhar too, since I applied patch drupal-make_rdf_module_pass_coder_review-1533380-15.patch, before starting
Comment #25
Jalandhar CreditAttribution: Jalandhar commentedComment #27
Jalandhar CreditAttribution: Jalandhar commentedHi Scor and damn,
Regarding 3 and 4 of comment #21, coder review is giving error that longer than 80 cols. can you please suggest me on how I can proceed about it so that I can do all changes of comment #21 and update patch.
Comment #28
scor CreditAttribution: scor commentedindentation issue.
It should be runTestBasicCommentRdfaMarkup() instead of runtestBasicCommentRdfaMarkup(). runTest should be spellt with uppercase T since Test is the second word in that CamelCase method name.
the method should be runTestAttributes() like it was in #15.
no need for a new line here, this is a regular function in a .module file (not a class).
phpunit.xml should not appear in the patch.
All the other changes such as the new line and $this->baseUri are good though. I would recommend to restart from the patch #15 and then apply your new line fixes (except the one in rdf.module and rdf.api.php and $this->baseUri.
Comment #29
scor CreditAttribution: scor commentedYou and dman are right, let's leave them as you have put them in multiple lines, in other words, please ignore what I said in 3 and 4 of comment #21.
Comment #30
Jalandhar CreditAttribution: Jalandhar commentedThanks scor and damn.
Regarding extra CR that I have memntioned in comment #15, the below code is working as said by damn.
Updating the patch with all required changes mentioned by you.
Comment #31
Jalandhar CreditAttribution: Jalandhar commentedComment #32
dman CreditAttribution: dman commentedThanks for your patience @jalandhar! Progress is being made and the end is in sight here!
Comment #33
Jalandhar CreditAttribution: Jalandhar commentedHi dman and scor,
Please review the patch attached in comment #30.
Comment #34
TravisCarden CreditAttribution: TravisCarden commentedThis is looking really good, all. A few comments:
There are a number of lines of commented out code in this patch. Since the spirit of the 80 character limit for comments is to make prose readable, and since making these changes will just create extra work in the future when the lines are un-commented, let's remove these changes and treat any warnings/errors as false positives.
I don't know of any precedent for splitting
@var
directives onto multiple lines. I feel like there's an implicit expectation of the opposite. Let's treat this as a false positive and remove these changes from the patch. Anybody want to file an issue against Coder?Comment #35
dman CreditAttribution: dman commented@Jalandhar - I have to apologise that you are getting mixed messages.
We have a conflict between getting a green light from the automation vs, as @TravisCarden says, the spirit of the 80 col limit is for readability - which clearly loses here.
If it were a local project I'd shrug and leave it as a false positive, but if this task is part of a global code coverage thing, it's annoying.
I dunno.
Comment #36
Jalandhar CreditAttribution: Jalandhar commented@damn: no problem...:P
@Travis carden:
I have done the changes related to all points mentioned by you in comment #34 but coder is returning errors/warnings because of changes related to 1,2 and 3. I have listed those errors in the attached file rdf_coder_errors.txt.
Please review the patch attached.
Comment #37
Jalandhar CreditAttribution: Jalandhar commentedChanging status to needs review.
Comment #38
Jalandhar CreditAttribution: Jalandhar commentedChanging Assignee to Unassigned
Comment #39
scor CreditAttribution: scor commentedSee #2188889-23: Support RDFa output in link field formatter for more things to fix.
Comment #40
xjmThanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.
Comment #41
tatarbjClosing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.