API page: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Plugi...

The documentation contains two occurrences where the Get class is referenced by a lower case get at the start of a sentence - when it should be Get.

In the following example code, @foo should be quoted; '@foo' and the parameters below process: should be indented 2 spaces.

process:
@foo:
  plugin: machine_name
  source: baz
bar:
  plugin: get
  source: '@@@foo'
CommentFileSizeAuthor
#21 fix-quotes-in-example-code-2941318-21-00B.patch1.8 KBapaderno
#18 interdiff-2941318-9-11.txt1 KBchiranjeeb2410
#18 interdiff-2941318-11-15.txt817 byteschiranjeeb2410
#15 incorrect-documentation-formatting-2941318-15.patch1.8 KBdavewilly
#11 incorrect-yml-formatting-2941318-11.patch1.28 KBdavewilly
#9 incorrect-yml-formatting-2941318-9.patch609 bytesdavewilly
#5 2941318-5.patch402 byteschiranjeeb2410
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davewilly created an issue. See original summary.

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
Issue summary: View changes
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: +Novice
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@davewily, should the change be made only for his chunk of the code?

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Active » Needs review
FileSize
2941318-5.patch402 bytes

@davewilly,

Attached patch as required. Review accordingly.

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good, you beat me to it :)

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@davewilly,

Thanks for the review :)

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented
Status: Reviewed & tested by the community » Needs review

The issue as written seems ok, however, shouldn't '@foo' and 'bar' be sub-keys of 'process'?

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
FileSize
incorrect-yml-formatting-2941318-9.patch609 bytes

Good spot @cilifen, patch updated.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Yes, those are sub-keys, as in the other examples.

I would also change the following sentence, which can just be more straight.

Because of this, if your source or destination property actually starts with a @ you need to double those starting characters up. This means that if a destination property happens to start with a @ and you want to refer it, you'll need to start with three @ characters -- one to indicate the destination and two for escaping the real @.

I would rather write it as:

Because of this, if the source or destination property actually starts with a @, that character must be escaped with @@.
The referenced property becomes, for example, @@@foo.

Also, get is used twice instead of the class name, which is Get.

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
FileSize
incorrect-yml-formatting-2941318-11.patch1.28 KB

Patch updated with adjusted description.

Isn't the lower case get correct since it references the annotation id?

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

The page is about the Get class, so it should say Get. I am not sure classes has been referenced with its annotation ID, in the documentation.

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented

You are referring to the lower case get in the copy not the example code? e.g.

The get plugin returns the value of the property given by the "source" configuration key.

These references should all be replaced with Get ?

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Yes, that's correct. The example code is fine; it's just what the sentences use to refer the class that should be changed, IMO.

I am fine as it is too, though. Actually, The get plugin is fine, but not get alone as used in the other sentence. (get also supports a list of source properties.)

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
FileSize
incorrect-documentation-formatting-2941318-15.patch1.8 KB

Updated to make lower case get references at start of sentences, uppercase.

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented

I have checked the patch, all the above mentioned points have been corrected.

shubhangi1995’s picture

shubhangi1995
Location GURGAON
CreditAttribution: shubhangi1995 commented
Status: Needs review » Reviewed & tested by the community
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
FileSize
interdiff-2941318-11-15.txt817 bytes
interdiff-2941318-9-11.txt1 KB

Patch seems to address all mentioned points correctly. However, adding interdiffs, as it just makes the
job easier.

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented
Issue tags: +Needs issue summary update

If someone else comes around to commit this they'll not understand the changes being made in relation to the issue title and summary, which are different.

davewilly’s picture

davewilly CreditAttribution: davewilly as a volunteer commented
Title: @foo should be quoted in example » Incorrectly formatted documentation in Get migrate process plugin
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
FileSize
fix-quotes-in-example-code-2941318-21-00B.patch1.8 KB

The patch applies to Drupal 8.6 without changes.

cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Version: 8.4.x-dev » 8.6.x-dev
Issue tags: -Needs issue summary update
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs review » Reviewed & tested by the community

Patch suffices all the changes required against latest core. RTBC for me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: fix-quotes-in-example-code-2941318-21-00B.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Reviewed & tested by the community

Testbot issues.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

The updated docs look good.

Committed and pushed a31892acd2 to 8.6.x and a52fc43666 to 8.5.x. Thanks!

  • alexpott committed a31892a on 8.6.x 
    Issue #2941318 by davewilly, chiranjeeb2410, kiamlaluno: Incorrectly...

  • alexpott committed a52fc43 on 8.5.x 
    Issue #2941318 by davewilly, chiranjeeb2410, kiamlaluno: Incorrectly...

Status: Fixed » Closed (fixed)

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