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.
Five years ago Steven added some 'vancode' functions for comment threading: #48239: Comment thread coding is inefficient. I love the famous last words there: 'I'm scared of anyone but Steven maintaining this if it stays as-is...'
Heine mentions that the origin of 'vancode' is that Steven dreamed it up in Vancouver: http://groups.drupal.org/node/12038#comment-39848
At the minimum we need to rename these functions so they begin with 'comment_'.
Comment | File | Size | Author |
---|---|---|---|
#22 | vancode-alphadecimal-1019928-15.patch | 4.09 KB | xjm |
#17 | 1019928-vancode-12.patch | 3.88 KB | aspilicious |
#15 | vancode-alphadecimal-1019928-15.patch | 4.09 KB | xjm |
#13 | vancode-alphadecimal-1019928-12.patch | 4.1 KB | xjm |
#3 | 1019928-vancode.patch | 2.27 KB | aspilicious |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedSubscribe lol
Comment #2
xjmTagging.
Comment #3
aspilicious CreditAttribution: aspilicious commentedStarting patch. I couldn't come up with a good name. So i added some info saying its a drupalism and a link to the issue that introduced it.
Comment #4
aspilicious CreditAttribution: aspilicious commentedcrosspost
Comment #5
joachim CreditAttribution: joachim commentedThe patch is definitely an improvement on the current situation!
(I do wonder whether we could replace the system with something better though...)
Comment #6
xjmThe substitution of "2" for "to" is a bit un-Drupaly. I'd suggest:
comment_int_to_alphadecimal()
and
comment_alphadecimal_to_int()
.Then we can kill the comment explaining wtf a vancode is.
"These alphadecimal codes can be sorted as strings without altering numerical order."
This is confusing. While we're cleaning it up, can we add
comment_increment_alphadecimal()
as a human-readable wrapper for this?Comment #7
aspilicious CreditAttribution: aspilicious commentedPeople still would google for alphadecimal and feeling unlucky
Comment #8
joachim CreditAttribution: joachim commentedI dunno, it got me http://en.wikipedia.org/wiki/Base_36 as the top result :D
Maybe add a few lines of explanation in the docs for comment_int_to_alphadecimal()?
Comment #9
aspilicious CreditAttribution: aspilicious commentedWell it is base 36 with a digit in front of it...
Comment #10
xjmThe word "alphadecimal" is an actual word, and lots of people know what it means. The word "vancode" is made up. :)
"Consists of a leading character indicating length, followed by N digits in base 36 (alphadecimal). These codes can be sorted as strings without altering numerical order."
And then put an
@see
to this function in the docblock of every function that calls it.Comment #11
xjmThere's also a reference we missed in
comment.install
.Comment #12
catchOh wow.
Comment #13
xjmComment #15
xjmI fail at M%.
Comment #16
xjmComment #17
aspilicious CreditAttribution: aspilicious commentedOk what about this patch?
Comment #18
xjm#17: Did you see #15 ? :)
Comment #19
aspilicious CreditAttribution: aspilicious commentedARGH crosspost :p.
#15 is a bit better. ;)
Comment #20
robertDouglass CreditAttribution: robertDouglass commentedSo sad. Vancode is a tiny bit of our history.
Comment #21
aspilicious CreditAttribution: aspilicious commentedDo you understand the problem with it Robert?
Its part of Drupal history but not part of everyones drupal history (if that makes sense).
Comment #22
xjmRe-uploading so it's clearer which patch to review.
Comment #24
xjm*scooby confused noise*
I just ran this test locally with latest pull, patch applied, and it doesn't fail.
Comment #25
xjm#22: vancode-alphadecimal-1019928-15.patch queued for re-testing.
Comment #26
kika CreditAttribution: kika commentedTag as #drupalhistory, please
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like the patch passed. Do you need user testing?
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedI also think this is without much merit. Must we exchange our history for dry toast? Do you really get pleasure from the term alphadecimal? Some things are better left as is.
Comment #29
rickmanelius CreditAttribution: rickmanelius commentedI applied the patch, created a node, and then proceeded to test threaded comments down 3 levels and everything is checking out.
Is there anything specific that needs testing beyond that? If not, it works for me. If there are any other considerations, let me know and I can test for them.
Comment #30
aspilicious CreditAttribution: aspilicious commentedMoshe I'm not sure...
Some developers try to google vancode and that is the problem.
We need committer feedback on this...
Comment #31
Jody LynnOh come on. Code is not a good place for nostalgia.
Comment #32
rickmanelius CreditAttribution: rickmanelius commentedUsing google's keyword tool, this word is only searched 320 times a month.
Incidentally, this issue is result number 2. http://www.google.com/search?q=vancode
So if anyone googled it, there would be a pretty good chance they'd find out that it was removed.
Comment #33
xjmSo every single month, 320 Drupal developers are confused by it?
In any case, the current function names are not self-explanatory nor documented. They also violate our naming conventions in several ways: http://drupal.org/coding-standards#naming
Comment #34
tstoecklerI totally support this patch. I spent a couple of hours in comment.module recently and if I hadn't known of this issue I would have been number 321 this month. The point is not that alphadecimal is self-documenting. Vancode sounds like something specific that you should know about if you're using the function. Like when I started programming I didn't know what an integer was, so I looked that up and everything was fine. Looking up vancode gets you nowhere, but looking up alphadecimal works fine. That's the point of this patch.
Comment #35
grendzy CreditAttribution: grendzy commentedIt's been a while, but my first encounter with "vancode" was pretty confusing too. I eventually found a comment on g.d.o explaining its origins, but only after crawling around in the dark for an hour or so.
This piece of history does not have significace for me personally, but if you really want to preserve it then we ought to at least explain the Vancouver / Unconed "legend" in the documentation.
Comment #36
Dries CreditAttribution: Dries commentedI'm comfortable committing this patch (and to throw away our vancode history).
Comment #37
rickmanelius CreditAttribution: rickmanelius commentedI'd say we get rid of it. The small lost in history will be outweighed by the gain in understanding and clarity moving forward.
Comment #38
catchSince Dries chimed in already I'll let him do the honours on this one.
I'm hopeful we can work on http://drupal.org/project/entity_tree and comment module won't need to maintain its own tree API any more, so this code could end up getting retired during Drupal 8 anyway.
Comment #39
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Comment #40
Dries CreditAttribution: Dries commentedComment #42
xjmI went to reference the change notification for this issue and discovered there wasn't one. Oops! The change notification should be pretty straightforward--good novice task.
Comment #43
TransitionKojo CreditAttribution: TransitionKojo commentedI'm working on a Change Notification at http://drupal.org/node/1439500
Comment #44
xjmI reviewed the change notification with Transition in IRC. Looks great!