Hey! Great module!

I was using it recently and realized this module didn't have a README file, and although it may not be much I figured this was an opportunity for me to give back for all the help I've received through using this module. So I'll put one together for you!

I'll have one together shortly for your review. :-)

Problem/Motivation

Missing README.txt

Proposed resolution

Create a README.txt

Remaining tasks

  1. Create README patch


  2. Review README patch

    3.
  3. Commit README patch

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#18 interdiff_13-18.txt3.98 KBdani3lr0se
#18 addressfield-readme-2653162-18.patch2.88 KBdani3lr0se
#17 interdiff_13-17.txt3.46 KBdani3lr0se
#17 addressfield-readme-2653162-17.patch2.88 KBdani3lr0se
#13 addressfield-README.txt-2653162-13.patch3.27 KBPatrick Storey
#12 addressfield-README.txt-2653162-12.patch3.33 KBPatrick Storey
#9 addressfield-README.txt-2653162-9.patch2.86 KBPatrick Storey
#6 addressfield-README.txt-2653162-6.patch2.86 KBPatrick Storey
#3 addressfield-README.txt-2653162-3.patch2.92 KBPatrick Storey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Patrick Storey created an issue. See original summary.

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
Version: 7.x-1.2 » 7.x-1.x-dev
Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
FileSize
addressfield-README.txt-2653162-3.patch2.92 KB

Here is the README file. Feel free to give it a read through, hopefully I got in everything there that you would want!

I used this https://www.drupal.org/node/2181737 as instructions and a template to make it.

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
Status: Active » Needs review
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Thanks for this. Here's some feedback.

  1. +++ b/README.txt
@@ -0,0 +1,58 @@
+  ¶

    Extraneous space 

  2. +++ b/README.txt
@@ -0,0 +1,58 @@
+The Address Field defines a new field type to store international postal addresses, implementing a subset of the top-level address elements defined in the xNAL standard. Can you put this in English and/or dumb it down?

    Remove "Can you put this in English and/or dumb it down?"

    I assume that was a note. 

  3. +++ b/README.txt
@@ -0,0 +1,58 @@
+The module has no menu or modifiable settings. There is no configuration. When enabled, the module will provide features such as a field type called postal address that you can add to your content type. Address Field defines a new field type to store international postal addresses and should work on any entity type. ¶

    Extraneous space at end of line

Note, if you review the patch using dreditor, you'll be able to see the spaces easier.

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
FileSize
addressfield-README.txt-2653162-6.patch2.86 KB

Thanks for the review!

Ah yes... that was a comment that I didn't notice snuck in there.

And I've removed the other spaces as well.

New patch uploaded!

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Whoops. Found one more nitpick.

+++ b/README.txt
@@ -0,0 +1,58 @@
+Address Field should work on any entity type, but if you use it on an entity type that can be created or edited by anonymous users (such as a user account field present on the registration form), you should ensure pages where the form is present are not cached by Drupal. Drupal's core page caching and its AJAX form refresh used by this module do not work together.The recommended solution for now is to use the Cache Exclude module to tell Drupal which pages not to cache, such as user/register.

Needs space between "together." and "The"

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
FileSize
addressfield-README.txt-2653162-9.patch2.86 KB

Change made.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs work » Reviewed & tested by the community

Wow! So fast. Thanks. The text looks good to me.

Unfortunately, when I applied the patch, I got this:

[mac:kristen:addressfield]$ patch -p1 < addressfield-README.txt-2653162-9.patch 
patching file README.txt
patch: **** malformed patch at line 62:
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Reviewed & tested by the community » Needs work
Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
FileSize
addressfield-README.txt-2653162-12.patch3.33 KB

Interesting. Okay I downloaded a fresh code base and got that error.

Re-made the patch, and this one should apply no problem.

Ah one second I see it re-added the default spacing at it was remade. Let me make those adjustments.

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
FileSize
addressfield-README.txt-2653162-13.patch3.27 KB

Updated patch file.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs work » Reviewed & tested by the community

The patch in #13 worked for me. The text looks ok to me. Marking RTBC. Thanks.

Patrick Storey’s picture

Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commented
Issue summary: View changes
volkswagenchick’s picture

volkswagenchick
she/her
Primary language English
Location San Francisco Bay Area
CreditAttribution: volkswagenchick at Hook 42 commented

I looked over the patch in comment 13 and it looks good to me as well.

dani3lr0se’s picture

dani3lr0se CreditAttribution: dani3lr0se at Hook 42 commented
Assigned: Patrick Storey » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
addressfield-readme-2653162-17.patch2.88 KB
interdiff_13-17.txt3.46 KB

Here is a new patch that adds line breaks at 80 characters to align with Drupal standards, along with an interdiff to show the differences. Thanks for the work on this patch everyone.

dani3lr0se’s picture

dani3lr0se CreditAttribution: dani3lr0se at Hook 42 commented
FileSize
addressfield-readme-2653162-18.patch2.88 KB
interdiff_13-18.txt3.98 KB

Just realized I was so excited that I actually successfully made an interdiff I messed up my patch. So here is a proper patch, that actually adds line breaks at 80 characters for all the lines that were longer than 80 characters.

volkswagenchick’s picture

volkswagenchick
she/her
Primary language English
Location San Francisco Bay Area
CreditAttribution: volkswagenchick at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

Patch applies locally and the reformatting looks good. Thanks for the re-roll.

marking RTBC

Sonal Gyanani’s picture

Sonal Gyanani CreditAttribution: Sonal Gyanani at Dotsquares Ltd. commented
Status: Reviewed & tested by the community » Needs work

Sorry for the inconvenience checking it for drupal latest version but it's for drupal 7

Sonal Gyanani’s picture

Sonal Gyanani CreditAttribution: Sonal Gyanani at Dotsquares Ltd. commented
Status: Needs work » Reviewed & tested by the community