Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Running coder on this module shows a variety of issues, as well as many @ignore
statements used to ignore improper formatting to align text - this isn't a Drupal standard, and should be corrected to bring the module code up to date with current Drupal standards.
Comment | File | Size | Author |
---|---|---|---|
#16 | advagg-2744395-15-coder-cleanup.patch | 63.93 KB | mikeytown2 |
#11 | advagg-2744395-10-use-strict-js.patch | 5.64 KB | mikeytown2 |
#7 | advagg-2744395-5-fix-eslint-js-coder-warnings.patch | 6.87 KB | mikeytown2 |
Comments
Comment #2
aburke626Patch to correct code standards issue and remove unneeded
@ignore
statements.A few remaining warnings:
These seemed to be used properly, as logging functions, but I figured I would put it out there if anyone wanted to re-factor them to get rid of the warnings.
Comment #3
aburke626Comment #4
mikeytown2 CreditAttribution: mikeytown2 commentedNo patch has been attached.
In term of kprint_r its being used to generate a nicely formatted error message. I don't see why this is an issue other than coder being extra cautious as you can get a call to undefined function error if you don't do proper checks. I can copy what I have inside of httprl but that would just add more lines of code just to make coder happy.
The dprint_r is used because I mirror the devel module's query log output as that js doesn't work if using defer.
I've been using http://pareview.sh/pareview/httpgitdrupalorgprojectadvagggit-7x-2x and it looks like they added in some js checking so I'll work on that as I'm guessing your patch has to do with php :)
Comment #5
aburke626Woops, uploaded the patch this time.
Comment #6
mikeytown2 CreditAttribution: mikeytown2 commentedThis isn't meant to be read but copy pasted so going over 80 is better.
Splitting up a url is a bad idea. Going over 80 is better for copy/paste.
This needs to be copy pasted into .htaccess without any modifications. This can not be altered.
This code still work?
This is just trying to make coder happy. For core hooks I agree that duplication isn't needed; for 3rd party hooks it's good to have the documentation in code as https://api.drupal.org/api/drupal/7.x doesn't contain this module. There are other places where this happens; only mentioning it here.
I think this is less readable; now you have to scroll.
Multi-line if statements; I think the way it's currently written is easier to read, but this change doesn't affect readability that much so I think I'd be ok with all of these changes (the bulk of this patch).
I think having the comments match the if statement hierarchy makes it easier to read. Not willing to do this change.
Extending core hooks to do more than what they originally did; documenting this in doxygen format is the right thing to do. Not willing to do this change.
Wrap this in @code @endcode. Good idea!
The array should be fully multi-line with a comma after url().
This change can't be done. The text needs to be ran through t().
This can just be removed as magic has an api file http://cgit.drupalcode.org/magic/tree/magic.api.php?h=7.x-1.x
The note about the $magic_settings being ready only and how it can't be in the return array should be kept though. Put the read only at the top and the part about the return at the bottom above return.
Comment #7
mikeytown2 CreditAttribution: mikeytown2 commentedjs changes. coder will still complain about the minified js though.
Comment #9
mikeytown2 CreditAttribution: mikeytown2 commented#7 has been committed; setting issue to needs work from the comments in #6.
Comment #11
mikeytown2 CreditAttribution: mikeytown2 commentedLooks like use strict can be single or double quotes. http://stackoverflow.com/questions/5214391/javascript-can-ecmascript-5s-... This patch addresses this; it's also been committed.
Comment #12
mikeytown2 CreditAttribution: mikeytown2 commented@aburke626
Any update on the issues I brought up in #6?
Comment #13
mikeytown2 CreditAttribution: mikeytown2 commented@aburke626
Any update on the issues I brought up in #6?
Should I unassign you from this issue?
Comment #14
mikeytown2 CreditAttribution: mikeytown2 commentedunassigning aburke626 due to lack of feedback. Will work on the changes suggested and issue credit where due.
Comment #15
mikeytown2 CreditAttribution: mikeytown2 commentedRe-roll of #5 with the comments from #6 taken into account.
Comment #16
mikeytown2 CreditAttribution: mikeytown2 commentedComment #17
mikeytown2 CreditAttribution: mikeytown2 commentedComment #22
mikeytown2 CreditAttribution: mikeytown2 commented#16 has been committed. Thanks for bringing this up!
Comment #23
mikeytown2 CreditAttribution: mikeytown2 commented