Add API documentation to the MachineName process plugin.

CommentFileSizeAuthor
#16 Added-documentation-2845484-16.patch1.4 KBjofitz
#16 interdiff-13-16.txt638 bytesjofitz
#13 Added-documentation-2845484-12.patch1.74 KBMaskyS
#6 2845484.patch1.73 KBWinthropian
#2 2845484-1.patch1.36 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
2845484-1.patch1.36 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: Unassigned » phenaproxima

Self-assigning for review.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MachineName.php
@@ -11,13 +11,26 @@
+ * The machine_name process plugin takes the source value, runs it over the

    s/source/input. And let's say either "runs it through the transliteration service" or "transliterates it". 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MachineName.php
@@ -11,13 +11,26 @@
+ * or a letter with an underscore and removes duplicate underscores. It's very
+ * often followed by one of the deduplication plugins.

    Not sure if the last sentence is helpful or confusing. Maybe we should axe it?

Winthropian’s picture

Winthropian CreditAttribution: Winthropian as a volunteer commented
Status: Needs work » Needs review
Issue tags: +SprintWeekend2017
FileSize
2845484.patch1.73 KB

Refined text

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

I reviewed this carefully in person at Boston sprint weekend. RTBC assuming the tests pass (and why wouldn't they). Thanks, @Winthropian!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented 
+++ b/core/modules/migrate/src/Plugin/migrate/process/MachineName.php
@@ -11,13 +11,28 @@
+ * the transliteration service. This makes the source value lowercase, ¶
+ * replaces anything that is not a number or a letter with an underscore, ¶
...
+ * Letters will have language decorations and accents removed. ¶

Trailing whitespace.

Leaving RTBC in the hope that that can be fixed on commit.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev

Straightforward code documentation improvements can always go into any patch release, alpha, beta, or RC, so please always file them against the production branch (currently 8.3.x). Thanks!

xjm’s picture

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

This patch has some trailing whitespace that needs to be cleaned up.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Or what @tstoeckler said, yes. Sorry, missed the comment. In general, committers editing things on commit is undesirable, and in the case of this patch, which is not urgent, the patch author can learn from it for future patches also. :)

MaskyS’s picture

MaskyS CreditAttribution: MaskyS at Google Code-In commented
Status: Needs work » Needs review

Removed whitespaces.

MaskyS’s picture

MaskyS CreditAttribution: MaskyS at Google Code-In commented
FileSize
Added-documentation-2845484-12.patch1.74 KB

Removed whitespaces.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Nothing objectionable here. Thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate/src/Plugin/migrate/process/MachineName.php
@@ -11,13 +11,28 @@
+ * If the value of foo in the source is áéí! then the destination value of bar
+ * will be aei_.

Hm, I thought I posted this feedback already but apparently I did not! Can we put single quotes around the example values? I got confused by it the first time I read it. Thanks!

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
interdiff-13-16.txt638 bytes
Added-documentation-2845484-16.patch1.4 KB

Added single quotes as requested by @xjm.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks made right. Thanks, @Jo Fitzgerald.

  • xjm committed 89a9510 on 8.4.x 
    Issue #2845484 by Jo Fitzgerald, quietone, Kifah Meeran, Winthropian,...
xjm’s picture

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

Thanks all! Committed to 8.4.x and cherry-picked to 8.3.x.

  • xjm committed 11d8dc9 on 8.3.x 
    Issue #2845484 by Jo Fitzgerald, quietone, Kifah Meeran, Winthropian,...

Status: Fixed » Closed (fixed)

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