This makes it easier to follow the flow of the code where sort() is called on the entity class with uasort().

Files: 
CommentFileSizeAuthor
#15 2084653-uasort-see-docs-15.patch2.72 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]
#8 2084653-8.drupal.uasort-see-ConfigEntityBase-sort.patch2.71 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]
#1 2084653.drupal.uasort-see-ConfigEntityBase-sort.patch2.71 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Makes sense to me. Documentation-only patch.

Status:Reviewed & tested by the community» Needs work

Sounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).

Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct? It wasn't obvious to me whether or not the references to the classes were correct, just looking at the patch. So I'd just like a bit of reassurance. :)

> Sounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).

Core has 183 occurrences of that!

ack --noheading --nobreak '@see Drupal\\' | wc -l
// Says 183
ack --noheading --nobreak '@see \\Drupal\\' | wc -l
// Says 626

I'll wait for a review of the classes before rerolling for the initial slash.

Yes, Core is often not following our standards, but we do have one that says if you use namespaces in docs, they *must* be fully qualified.
https://drupal.org/node/1354#classes

The previous patch was reviewed, so you might as well fix it now? :)

> Yes, Core is often not following our standards

:D

> Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct?

I mean that bit. Makes more sense to wait for any comments on that front, and reroll then?

Yes, I checked them all :) Details as follows:

ConfigEntityListController is a generic list controller for ConfigEntityBase objects, so ConfigEntityBase::sort() is correct.

BlockListController lists Block entities, which have their own overridden sort function, so Block::sort() is correct.

LanguageListController lists Language entities, which do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.

RoleStorageController handles Role entities, which again do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.

Status:Needs work» Needs review
StatusFileSize
new2.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]

Updated patch.

I've removed the full stops from the end of the @see lines, as AFAICT that's the correct format.

Status:Needs review» Reviewed & tested by the community

Looks good. @see needs no trailing stop, though core does not always conform to this.

$ git grep "@see.*[^\\.]$" | wc -l
2702
$ git grep "@see.*\\.$" | wc -l
136

Status:Reviewed & tested by the community» Needs review

Should we even use @see in inline documentation? It has a specific use in /** */ comments because those turn into links and a See Also section... ??? It's probably better to just say "See blah blah blah" in in-line docs?

Status:Needs review» Reviewed & tested by the community

We use it in enough other places already:

$ git grep "// @see" | wc -l
154

16 inline @see references to classes:

$ git grep "// @see.*Drupal.*::"
core/includes/bootstrap.inc:  // @see Drupal\simpletest\TestBase::prepareConfigDirectories()
core/lib/Drupal/Core/Config/CachedStorage.php:    // @see Drupal\Core\Cache\CacheBackendInterface::getMultiple()
core/lib/Drupal/Core/Entity/DatabaseStorageController.php:        // @see Drupal\Core\Entity\EntityInterface::__construct()
core/lib/Drupal/Core/Entity/DatabaseStorageController.php:      // @see Drupal\Core\Entity\EntityInterface::__construct()
core/lib/Drupal/Core/StreamWrapper/PublicStream.php:      // @see Drupal\simpletest\WebTestBase::setUp()
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php:        // @see \Drupal\Core\TypedData\Validation\PropertyContainerMetadata::accept();
core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.php:      // @see Drupal\Core\Entity\FeedStorageController::postSave()
core/modules/ckeditor/js/ckeditor.admin.js:      // @see \Drupal\ckeditor\Plugin\editor\editor\CKEditor::settingsForm.
core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.php:    // @see Drupal\simpletest\TestBase::error()
core/modules/config/lib/Drupal/config/Tests/ConfigLocaleOverrideWebTest.php:    // @see \Drupal\Core\PathProcessor::processInbound()
core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUIRouteTest.php:    // @see \Drupal\field_ui\FieldOverview::getRegions()
core/modules/filter/filter.module:  // @see \Drupal\filter\Entity\FilterFormat::save()
core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php:      // @see Drupal\Core\Entity\EntityInterface::__construct()
core/modules/serialization/tests/serialization_test/lib/Drupal/serialization_test/SerializationTestEncoder.php:    // @see Drupal\serialization_test\SerializationTestNormalizer::normalize().
core/modules/statistics/lib/Drupal/statistics/Tests/Views/IntegrationTest.php:    // @see \Drupal\statistics\Tests\StatisticsLoggingTest::testLogging().
core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php:    // @see Drupal\simpletest\WebTestBase::xpath()

Status:Reviewed & tested by the community» Needs review

But "we're doing it a lot" doesn't mean "it's a good idea to do it more" or "it's our standard". For instance, we're using the word "id" all over documentation in place of "ID", but it's still incorrect every time we do it.

My question is whether using @see in inline docs is a good idea, since "See" would be just as clear and is plain English, and our documentation standards generally say we should use plain English for documentation, and @ tags in /** */ headers, right?

Isn't that out of scope for this issue? Shouldn't we just get this improvement in, and then address that (and all other uses of // @see) in a followup?

Um... Let's follow our current standards in this patch, which say to write inline docs in plain English, and if you want to change the standards so that we encourage using @tags in inline docs, you can do that in another issue? :)

StatusFileSize
new2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]

Title:add a @see to ConfigEntityBase::sort() where used by uasortAdd inline comments referencing ConfigEntityBase::sort() where used by uasort

I guess the old title is inaccurate :)

Status:Needs review» Reviewed & tested by the community

Thank you. This looks fine to me.

Status:Reviewed & tested by the community» Fixed

Thanks again- committed to 8.x.

Status:Fixed» Closed (fixed)

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