Part of meta-issue #1310084: [meta] API documentation cleanup sprint

CommentFileSizeAuthor
#19 clean_up_api_docs_profiles-1355362-19.patch2.83 KBbartlantz
#14 clean_up_api_docs_profiles_directory-1355362-14.patch2.86 KBbartlantz
#9 clean_up_api_docs_profiles_directory-1355362-9.patch2.87 KBbartlantz
#8 clean_up_api_docs_profiles_directory-1355362-8.patch2.87 KBbartlantz
#4 clean_up_api_docs_profiles_directory-1355362-4.patch2.87 KBbartlantz
#2 clean_up_api_docs_profiles_directory-1355362-3.patch2.87 KBbartlantz
#1 clean_up_api_docs_profiles_directory-1355362-2.patch3.06 KBbartlantz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
Status: Active » Needs review
FileSize
clean_up_api_docs_profiles_directory-1355362-2.patch3.06 KB

Here's a patch to clean up API docs in the profiles directory.
Thanks,
Bart

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
FileSize
clean_up_api_docs_profiles_directory-1355362-3.patch2.87 KB

Here's a better patch, in the previous one I didn't document the hook definitions correctly. This patch replaces the one above and adds this line to the hook definitions:
Implements hook_form_FORM_ID_alter() for install_configure_form()

Thanks,
Bart

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work 
+++ b/profiles/testing/testing.profile
@@ -1,2 +1,6 @@
+ * Tests for installation profiles.

That's not correct. It's more the other way around.
"Installation profile for tests."

-11 days to next Drupal core point release.

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
Status: Needs work » Needs review
FileSize
clean_up_api_docs_profiles_directory-1355362-4.patch2.87 KB

Aha, that makes more sense! Okay, I fixed the description of the testing.profile. It now says, "Installation profile for tests."

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Cool stuff!
Thanks for the quick re-roll.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I can't open the last patch o_O

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs work

uh oh, me neither. bartlantz I hope you still have a copy of it...

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
FileSize
clean_up_api_docs_profiles_directory-1355362-8.patch2.87 KB

Yes, I still do, here it is.

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
Status: Needs work » Needs review
FileSize
clean_up_api_docs_profiles_directory-1355362-9.patch2.87 KB

whoops, forgot to change status to needs review. Was the file unavailable on drupal or is there a problem with the patch?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

You don't need to upload it again when you forgot to change the status, it will test the last patch after changing the status.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

barlantz: it looks like Drupal.org lost the previous upload. Not sure why. Thanks for re-uploading.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

This looks fine, thanks!

webchick’s picture

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

I found a couple of minor things, which I could just fix on commit, but since this is your first(?) core patch, I wanted to give you the opportunity to do so. :)

+++ b/profiles/minimal/minimal.installundefined
@@ -1,9 +1,16 @@
+/**
+ * @file
+ * Install, update and uninstall functions for the the minimal install profile.
+ */
+

This actually adds one more newline than we need.

+++ b/profiles/minimal/minimal.profileundefined
@@ -1,7 +1,12 @@
+ * Enables modules and site configuration for a minimal site installation.
+ **/

one too many *s on the */ line.

+++ b/profiles/testing/testing.profileundefined
@@ -1,2 +1,6 @@
 
+/**
+ * @file

Here you've left a newline between <?php and /** but elsewhere you don't. It should be consistent.

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
Status: Needs work » Needs review
FileSize
clean_up_api_docs_profiles_directory-1355362-14.patch2.86 KB

Cool, thanks! Here's a patch with the fixes.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! The three issues identified in #13 (which I missed, sorry!) are fixed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: bartlantz » jhodgdon

Moving to Jennifer's RTBC pile. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Committed to 8.x! Moving to 7.x for backport.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Novice

Backporting this would be a good novice issue. Should be simple; I didn't see any D8-specific changes on a quick skim.

bartlantz’s picture

bartlantz CreditAttribution: bartlantz commented
Assigned: Unassigned » bartlantz
Status: Patch (to be ported) » Needs review
FileSize
clean_up_api_docs_profiles-1355362-19.patch2.83 KB

Here's the patch backported to 7.x.

thanks,
Bart

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Backport looks fine too. Thanks @bartlantz!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x -- Another API docs cleanup done! :)

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