html2text-remove-tags documentation

html2text-remove-tags documentation

[Deniz Dogan]

This is the definition of `html2text-remove-tags' in lisp/gnus/html2text.el:

(defun html2text-remove-tags (tag-list)
   "Removes the tags listed in the list `html2text-remove-tag-list'.
See the documentation for that variable."
   (interactive)
   (dolist (tag tag-list)
     (goto-char (point-min))
     (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) 
(point-max) t)
       (delete-region (match-beginning 0) (match-end 0)))))

As you can see, the documentation is clearly incorrect.  The function 
removes the tags in TAG-LIST, not `html2text-remove-tag-list'. 
Furthermore, the function is interactive which doesn't make sense the 
way it's written now.

So what should we do about this function?

I suggest we make TAG-LIST optional and defaulted to 
`html2text-remove-tag-list', i.e., (or tag-list 
html2text-remove-tag-list).  If called interactively, TAG-LIST should be 
a space-or-comma-separated string of tags to remove.

Such a definition could be:

(defun html2text-remove-tags (&optional tag-list)
   "Remove the tags in TAG-LIST.
If TAG-LIST is nil, use `html2text-remove-tag-list'.
If called interactively, "
   (interactive "MTags to remove: ")
   (setq tag-list (if (called-interactively-p 'any)
                      (split-string tag-list "[ ,]" t)
                    (or tag-list html2text-remove-tag-list)))
   (dolist (tag tag-list)
     (goto-char (point-min))
     (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) 
(point-max) t)
       (delete-region (match-beginning 0) (match-end 0)))))

This definition would not break any existing code as far as I can tell 
and both fixes and adds functionality.

What do you think?

Deniz

Re: html2text-remove-tags documentation

[Deniz Dogan]

On 2011-07-24 11:11, Deniz Dogan wrote:
> This is the definition of `html2text-remove-tags' in
> lisp/gnus/html2text.el:
>
> (defun html2text-remove-tags (tag-list)
> "Removes the tags listed in the list `html2text-remove-tag-list'.
> See the documentation for that variable."
> (interactive)
> (dolist (tag tag-list)
> (goto-char (point-min))
> (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) (point-max) t)
> (delete-region (match-beginning 0) (match-end 0)))))
>
> As you can see, the documentation is clearly incorrect. The function
> removes the tags in TAG-LIST, not `html2text-remove-tag-list'.
> Furthermore, the function is interactive which doesn't make sense the
> way it's written now.
>
> So what should we do about this function?
>
> I suggest we make TAG-LIST optional and defaulted to
> `html2text-remove-tag-list', i.e., (or tag-list
> html2text-remove-tag-list). If called interactively, TAG-LIST should be
> a space-or-comma-separated string of tags to remove.
>
> Such a definition could be:
>
> (defun html2text-remove-tags (&optional tag-list)
> "Remove the tags in TAG-LIST.
> If TAG-LIST is nil, use `html2text-remove-tag-list'.
> If called interactively, "
> (interactive "MTags to remove: ")
> (setq tag-list (if (called-interactively-p 'any)
> (split-string tag-list "[ ,]" t)
> (or tag-list html2text-remove-tag-list)))
> (dolist (tag tag-list)
> (goto-char (point-min))
> (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) (point-max) t)
> (delete-region (match-beginning 0) (match-end 0)))))
>
> This definition would not break any existing code as far as I can tell
> and both fixes and adds functionality.
>
> What do you think?
>

Bumping this hoping to get it in before the "hard" feature freeze.

Deniz

Re: html2text-remove-tags documentation

[Andreas Röhler]

Am 27.07.2011 03:43, schrieb Deniz Dogan:
> On 2011-07-24 11:11, Deniz Dogan wrote:
>> This is the definition of `html2text-remove-tags' in
>> lisp/gnus/html2text.el:
>>
>> (defun html2text-remove-tags (tag-list)
>> "Removes the tags listed in the list `html2text-remove-tag-list'.
>> See the documentation for that variable."
>> (interactive)
>> (dolist (tag tag-list)
>> (goto-char (point-min))
>> (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) (point-max) t)
>> (delete-region (match-beginning 0) (match-end 0)))))
>>
>> As you can see, the documentation is clearly incorrect. The function
>> removes the tags in TAG-LIST, not `html2text-remove-tag-list'.
>> Furthermore, the function is interactive which doesn't make sense the
>> way it's written now.
>>
>> So what should we do about this function?
>>
>> I suggest we make TAG-LIST optional and defaulted to
>> `html2text-remove-tag-list', i.e., (or tag-list
>> html2text-remove-tag-list). If called interactively, TAG-LIST should be
>> a space-or-comma-separated string of tags to remove.
>>
>> Such a definition could be:
>>
>> (defun html2text-remove-tags (&optional tag-list)
>> "Remove the tags in TAG-LIST.
>> If TAG-LIST is nil, use `html2text-remove-tag-list'.
>> If called interactively, "
>> (interactive "MTags to remove: ")
>> (setq tag-list (if (called-interactively-p 'any)
>> (split-string tag-list "[ ,]" t)
>> (or tag-list html2text-remove-tag-list)))
>> (dolist (tag tag-list)
>> (goto-char (point-min))
>> (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) (point-max) t)
>> (delete-region (match-beginning 0) (match-end 0)))))
>>
>> This definition would not break any existing code as far as I can tell
>> and both fixes and adds functionality.
>>
>> What do you think?
>>
>
> Bumping this hoping to get it in before the "hard" feature freeze.
>
> Deniz
>
>

Hi Deniz,


as the argument is mandatory, it will not take the wrong thing.
`html2text-remove-tag-list' is just a proposal for proper use. You may 
give any appropriate list to work with.

You could enhance the docu maybe writing:

"Removes the tags listed in TAG-LIST, see `html2text-remove-tag-list' 
for example."

Also it might be useful IMHO making it restrict to active region, 
introducing something like:

   (let ((beg (cond (beg)
                            ((region-active-p)
                             (region-beginning))
                            (t (point-min))))
                 (end (cond (end (copy-marker end))
                            ((region-active-p)
                             (copy-marker (region-end)))
                            (t (copy-marker (point-max))))))
etc.


Thanks,

Andreas

Re: html2text-remove-tags documentation

[Andreas Röhler]

>> Furthermore, the function is interactive which doesn't make sense the
>> way it's written now.
>>

Thats right.

then (&optional tag-list)

in connection with

(or tag-list html2text-remove-tag-list)))

should DTRT.

Would not restrict it to interactive use,
as leaving out the argument-spec speeds up writing.

Cheers,

Andreas





>> So what should we do about this function?
>>
>> I suggest we make TAG-LIST optional and defaulted to
>> `html2text-remove-tag-list', i.e., (or tag-list
>> html2text-remove-tag-list). If called interactively, TAG-LIST should be
>> a space-or-comma-separated string of tags to remove.
>>
>> Such a definition could be:
>>
>> (defun html2text-remove-tags (&optional tag-list)
>> "Remove the tags in TAG-LIST.
>> If TAG-LIST is nil, use `html2text-remove-tag-list'.
>> If called interactively, "
>> (interactive "MTags to remove: ")
>> (setq tag-list (if (called-interactively-p 'any)
>> (split-string tag-list "[ ,]" t)
>> (or tag-list html2text-remove-tag-list)))
>> (dolist (tag tag-list)
>> (goto-char (point-min))
>> (while (re-search-forward (format "\\(</?%s[^>]*>\\)" tag) (point-max) t)
>> (delete-region (match-beginning 0) (match-end 0)))))
>>
>> This definition would not break any existing code as far as I can tell
>> and both fixes and adds functionality.
>>
>> What do you think?
>>
>
> Bumping this hoping to get it in before the "hard" feature freeze.
>
> Deniz
>
>