Now this is just a complete mess:

Yuck.

I'm assuming that this is going to be *fairly* easy to straighten out, so optimistically marking as a novice patch. :)

CommentFileSizeAuthor
#7 sortbytitle3.patch1.37 KBoestrich
#5 system-simpletests-alpha.png54.77 KBpaulhenrich
#4 sortbytitle.patch1.33 KBoestrich
#4 sortbytitle2.patch1.35 KBoestrich
#2 sortbytitle.patch1.08 KBoestrich

Comments

oestrich’s picture

oestrich commented
Assigned: Unassigned » oestrich

Gonna have a go at this.

oestrich’s picture

oestrich commented
Status: Active » Needs review
StatusFileSize
new sortbytitle.patch1.08 KB

Here goes nothing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Here's a code-style review. Haven't tested the patch yet.

     );
+	
+	uasort($element, "_simpletest_sort_by_title");

1. You're introducing trailing whitespace on that first + line.
2. The uasort should be indented as far as the );. We use 2 spaces rather than tabs for indentation. See http://drupal.org/coding-standards

+function _simpletest_sort_by_title($a, $b){
+  if(!isset($a['#title']) || !isset($b['#title'])){
+    return 1;
+  }
+
+  return strcasecmp($a['#title'], $b['#title']);
+}
+

1. This function should have a line of PHPDoc to explain what it is for.
2. There should be a space after the ) and before the { in the function definition and if.
3. There should be a space between if and (

Curious. Why return 1; if those aren't set? And under what circumstances will the #title property not get set? http://api.drupal.org/api/function/system_sort_modules_by_info_name/7 looks similar to this and doesn't have that if (!isset(..)) stuff.

We could probably do with a couple of inline comments to explain what's happening here.

oestrich’s picture

oestrich commented
Status: Needs work » Needs review
StatusFileSize
new sortbytitle2.patch1.35 KB
new sortbytitle.patch1.33 KB

The if(!isset()) part is for the other parts of $element that are not arrays or include #title. There are lots of errors if this isn't included.

I returned 1 because 0 didn't work. Not the best reason, but hey it works.

The tabs were there because my editor hadn't been switched over to use spaces instead of tabs.

paulhenrich’s picture

paulhenrich commented
Status: Needs review » Needs work
StatusFileSize
new system-simpletests-alpha.png54.77 KB

Just applied the patch to my D7 install (-dev from drupal.org/project tonight, not cvs). It seems to work just fine. I've attached a screen capture.

cwgordon7’s picture

cwgordon7 commented

Code comments should start with a space and a capital letter and end with a period. (//Sorting $element by it's childrens #title attribute instead of by class name should be // Sorting $element by it's childrens #title attribute instead of by class name.. Comments also should wrap at 80 characters. Same thing applies for the other code comments).

"_simpletest_sort_by_title" - we typically use single quotes for consistency, so '_simpletest_sort_by_title' would probably be better. We use double quotes for cases such as "stuff with apos'trophes" and "stuff with $variable in it".

if (!isset($a['#title']) || !isset($b['#title'])){ - there should be a space between the control structure and the opening bracket, such as: if (!isset($a['#title']) || !isset($b['#title'])) {.

Other than code style issues, though, it looks good, and this change will definitely make SimpleTest more usable. :)

oestrich’s picture

oestrich commented
Status: Needs work » Needs review
StatusFileSize
new sortbytitle3.patch1.37 KB

Alright, third times the charm.

cwgordon7’s picture

cwgordon7 commented
Status: Needs review » Reviewed & tested by the community

Awesome, this is a great change, it's really irksome trying to find the test you're trying to run in an alphabetized list of tests. This passes tests, and works, and complies with coding standards. Thank you very much!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

w00t! This has bothered me for many months.

Thanks so much, oestrich! Committed to HEAD. :)

Status: Fixed » Closed (fixed)
Issue tags: -Novice

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.