Problem/Motivation

INSTALL.txt should be converted to Markdown, to match README.md. Also, they could probably do with a little make-over before going to beta. This issue is based on changes proposed in #2751397: Fix coding style to meet Drupal standards.

Proposed resolution

Convert INSTALL.txt to Markdown (and rename to INSTALL.md), review content of both files.

Remaining tasks

  • Patch
  • Review
  • Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 interdiff_5-8.txt11.84 KBphilltran
#8 stage_file_proxy-cleanup_doc_files-2909440-8.patch7.4 KBphilltran
#5 stage_file_proxy-cleanup_doc_files-2909440-5-merge-d8.patch9.09 KBeelkeblok
#5 stage_file_proxy-cleanup_doc_files-2909440-5-d8.patch9.66 KBeelkeblok
#4 stage_file_proxy-cleanup_doc_files-2909440-3-d8.patch6.49 KBeelkeblok
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eelkeblok created an issue. See original summary.

eelkeblok’s picture

eelkeblok
him/his
Primary language Dutch
Location Netherlands 🇳🇱
CreditAttribution: eelkeblok at One Shoe commented
Issue summary: View changes
eelkeblok’s picture

eelkeblok
him/his
Primary language Dutch
Location Netherlands 🇳🇱
CreditAttribution: eelkeblok at One Shoe commented
Assigned: Unassigned » eelkeblok
Status: Active » Needs work

These are the changes as proposed by 'mfernea' and 'Pavan B S' in #2751397: Fix coding style to meet Drupal standards. Please credit them when commiting.

Here are my review points for these changes.

  1. -STEPS TO INSTALL:
-=========
+## STEPS TO INSTALL

    I agree with the conversion from H1 to H2. However, I feel upper case in markdown is not really needed (specifically because it doesn't look as nice when converted to HTML) and I prefer underline style headings for h1 and h2 because it does a better job of emphasising the heading when reading source. 

  2. +## REQUIRED
 
-REQUIRED:
-=========
+### The origin website.
 
-The origin website.

    If we are going to change this, I would suggest to introduce a single heading "Configuration", with subheadings for each of the configuration values. Specify whether the configuration is optional per sub-heading. That way, it is also possible to write a few words about configuring through the admin interface (this installation guide seems to be largely based on the installation procedure from the very first versions of SFP for D6/D7; the fact it is even possible is completely ignored). 

  3. -INTRODUCTION
-------------
+## INTRODUCTION

    Like I said, I'm personally no great fan of the hash syntax. Therefore I wouldn't say this is a necessary change (same for the other changes of the same nature). 

  4.  $ drush en --yes stage_file_proxy
+```
 
 2. Configure connection to the source. This is available via the UI, at

    I was going to say this is an issue (although it might have been an existing one), because the Markdown preview tool I'm using starts a new list from point 2. I've tried a few online ones (including John Gruber's original web dingus) and it seems to go fine there. Still,apparently it is an issue with certain parsers, so I'll see if I can get it to behave better.

eelkeblok’s picture

eelkeblok
him/his
Primary language Dutch
Location Netherlands 🇳🇱
CreditAttribution: eelkeblok at One Shoe commented
FileSize
stage_file_proxy-cleanup_doc_files-2909440-3-d8.patch6.49 KB

Right. And the patch...

eelkeblok’s picture

eelkeblok
him/his
Primary language Dutch
Location Netherlands 🇳🇱
CreditAttribution: eelkeblok at One Shoe commented
FileSize
stage_file_proxy-cleanup_doc_files-2909440-5-d8.patch9.66 KB
stage_file_proxy-cleanup_doc_files-2909440-5-merge-d8.patch9.09 KB

Here's two patches that implement my proposed changes. The second one takes things a step further, it integrates both files into README.md, since there was some redundant information in both and it doesn't seem really necessary to have two separate files.

eelkeblok’s picture

eelkeblok
him/his
Primary language Dutch
Location Netherlands 🇳🇱
CreditAttribution: eelkeblok at One Shoe commented
Assigned: eelkeblok » Unassigned
Status: Needs work » Needs review
alonaoneill’s picture

alonaoneill CreditAttribution: alonaoneill at Hook 42 commented
Status: Needs review » Needs work

Patch has extra spaces that needs to be removed:

+++ b/README.md
@@ -17,42 +17,169 @@ making a copy of the production file in your development site. It makes it
+1.  Through the admin UI at Configuration > Stage File Proxy Settings ¶
...
+configure this within your settings.php or settings.local.php file. ¶
...
+An alternative is to make use of configuration synchronization combined with ¶
...
+That way, it is also possible to keep configuration for this module away from ¶
...
+image module handle it. This will speed up future requests for a different ¶
...
+For more information, see [/examples/sync_enable.drush.inc] from the Drush ¶

Extra spaces should be removed

philltran’s picture

philltran CreditAttribution: philltran commented
Version: 8.x-1.0-alpha3 » 8.x-1.x-dev
Status: Needs work » Needs review
FileSize
stage_file_proxy-cleanup_doc_files-2909440-8.patch7.4 KB
interdiff_5-8.txt11.84 KB

Here is a patch to update INSTALL.md. Looks like the README.md has been updated since patch #5. Since the README.md references INSTALL.md I went with keeping the two files separate versus merging INSTALL.md into README.md.

@alonaoneill I am not sure if I understood all of the extra spaces you pointed out. Please review and see if I missed something. Thanks.

Status: Needs review » Needs work

The last submitted patch, 8: stage_file_proxy-cleanup_doc_files-2909440-8.patch, failed testing. View results

philltran’s picture

philltran CreditAttribution: philltran commented
Status: Needs work » Needs review

Failed automated tests... because there are none. Tagging this back as needs review.

greggles credited Pavan B S.

greggles credited mfernea.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented

Adding credit for folks.

  • greggles committed 7ccf629 on 8.x-1.x authored by eelkeblok 
    Issue #2909440 by eelkeblok, philltran, mfernea, alonaoneill, greggles,...
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented
Status: Needs review » Fixed

I fixed the whitespace issues before committing/pushing.

Thanks everyone for your help with this!

Status: Fixed » Closed (fixed)

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