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.
Round 2. :)
I read through our entire code and found a bunch of things. A lot is simply code-style, missing words, inconsistent wrapping, etc.
Some words to ease review follow.
Comment | File | Size | Author |
---|---|---|---|
#16 | libraries-fix.patch | 1.48 KB | tstoeckler |
#10 | libraries-clean-up-3.patch | 38.72 KB | tstoeckler |
#8 | libraries-clean-up-2.patch | 35.43 KB | tstoeckler |
#2 | libraries-clean-up.patch | 34.18 KB | tstoeckler |
#1 | libraries-clean-up.patch | 0 bytes | tstoeckler |
Comments
Comment #1
tstoeckler....and the patch.
Comment #2
tstoeckler...ahhhhhhhh
Comment #3
tstoecklerPretty please... :)
The ordering before was:
- version callback
- version
- version arguments
now:
- version
- version callback
- version arguments
Only a change in the first sentence, the rest is wrapping.
In light of #864376: Loading of external libraries, I removed the word 'external' here.
I just cleaned this up a bit, and killed one if/else, "Hide deletions" probably helps here.
It was the only place using $name, so I inlined the variable.
and below:
Since we now have 'error' and 'error message' IMO it makes sense to make 'error message' more complete sentences.
I don't know why we had version 2, but it doesn't really make any sense.
The text below needed some updating, due to our updated testing method.
Powered by Dreditor.
Comment #4
sunAwesome!
Holy shit, I didn't add you to the README.txt yet?! I'm terribly sorry! SLOPPYME.
Mayhaps at least add a link to your d.o profile? Let's also change my link to my d.o profile, please.
Also, I've just seen that README.txt is not in UTF-8 yet, can you fix that, too?
We should double-check this and provide a valid regex example for matching a most common library version, as this was one of the initial integration problems I found.
For example, jQuery module uses:
Though that might be not an optimal example. Wysiwyg editor plugins contain better ones, I think.
Also, as visible in the pasted jQuery lines, we might want to document that it is a good idea to remember the different requirements for regex patterns in inline comments right above the pattern. I've introduced that pattern for Wysiwyg some time ago and it turned out to be really helpful, once you need to adjust the pattern for a new version.
d'oh :-D
Powered by Dreditor.
Comment #5
tstoecklerCopying some version regular expressions from Wysiwyg CVS for reference.
I still violently refuse to learn regular expressions properly :)
'@v([\d\.]+)@'
(v1.4.7)'@majorVersion[=:]["\'](\d).+?minorVersion[=:]["\']([\d\.]+)@'
(majorVersion:'3',minorVersion:'2.0.1') *'@Whizzywig v?([0-9]+)@'
(Whizzywig 60)'@version\s+([0-9a-z\.-]+)@'
(@version 0.5-rc1)'@version:\s([0-9\.]+)@'
(version: 2.8.2r1)'@version:\'(?:CKEditor )?([\d\.]+)(?:.+revision:\'([\d]+))?@'
(version:'3.0.1',revision:'4391' ) *'@^FCKeditor.prototype.Version\s*= \'([\d\.]+)@'
(FCKeditor.prototype.Version = '2.6.6' ;)'@([0-9\.]+)$@'
(WYSIWYG - jQuery plugin 0.6)'@([0-9\.]+)@'
(markItUp! 1.1.9)* This currently does not work with
libraries_get_version()
, because we only catch the first match.Comment #6
sunI'd suggest to change the current regex for this patch into:
and move a focused improvement of libraries_get_version() into a separate issue, including details of #5 and #4.
Comment #7
tstoecklerHere we go.
I noted the thing about libraries_get_version() merely for reference.
While I'm sure we'll find ways to improve it in the future, I was actually quite astonished to find out it *should* (famous last words...) work for 7 out of 9 editors.
< offtopic >I was also astonished to find out that WYSIWYG now supports 9 editors, by the way, I hadn't checked in for a while...< /offtopic >
Comment #8
tstoecklerDear lord...
Comment #9
tstoecklerOh, and:
My editor tells me the file is UTF-8, but I'm not entirely sure how to check/change that.
Comment #10
tstoecklerIn the meantime, when working on the tests for the callback issue, I noticed that the ordering of $expected and $library in some of the tests was a bit weird, from my point of view.
Comment #11
tstoecklerThose two lines should be switched around: The 'variants' key should be unset whether a variant was specified or not.
Can't be bothered right now to reroll just for that, in light of me still being unable to figure out the UTF-8 thing.
Powered by Dreditor.
Comment #12
sunHad some troubles to understand #11, but got it in the end. Changed those lines as proposed and also added a comment to explain the unset() for future reference (I'm relatively confident that we're going to revise libraries_load() and that variant handling soonish).
Thanks for reporting, reviewing, and testing! Committed to HEAD. Backported relevant changes to D6 (including README.txt).
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
Comment #13
lpalgarvio CreditAttribution: lpalgarvio commentedawesome work guys. congrats :)
Comment #14
tstoecklerOops. Sorry, that was an accidental change that I had on a local dev version, that wasn't supposed to sneak in the patch. I really hate the reference passing in libraries_detect_library() (yes, I know, I championed that...), but I deemed that to be an API change, which was not doable at the time. Now that we have a 1.0 version, though, we might as well make that change. I'm for it anyways.
But the change is not complete, it needs to change the calling of libraries_detect_library() everywhere including the tests and libraries_load() (!).
Sorry for that, I thought I had reverted that stuff properly before rolling the patch.
Powered by Dreditor.
Comment #15
sunNo big deal, since that change didn't go into 1.x anyway.
At first sight, the change made sense to me. Now that you mention it again, I don't think it's beneficial, since it complicates custom library code; i.e.,
Comment #16
tstoecklerWell, maybe it's best to discuss any API changes in another issue. Now that we're free to do that, I think I'll open one and write-up a few alternatives.
Anyway, here is a patch to fix our broken HEAD.
Tests are broken before and work with this patch.
Comment #17
tstoecklerComment #18
sunI left out this change, so it doesn't matter whether you assign the return value or expect the library to be altered by reference.
Thanks for reporting, reviewing, and testing! Committed to HEAD.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.