Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#11636 closed enhancement (fixed)

Refactoring of trac.notification

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: lowest Milestone: 1.1.2
Component: notification Version:
Severity: normal Keywords: refactoring
Cc: Branch:
Release Notes:
API Changes:

ListOption and Configuration.getlist support multiple separators, specified in a tuple or list.

Internal Changes:

Description (last modified by Ryan J Ollos)

Just a few refactorings I noticed while looking at code in the module: rjollos.git:t11636.

Attachments (0)

Change History (8)

comment:1 by Ryan J Ollos, 6 years ago

Description: modified (diff)
Status: newassigned

comment:2 by Ryan J Ollos, 6 years ago

Priority: normallowest
Summary: Refactoring trac.notificationRefactoring of trac.notification

comment:3 by Jun Omae, 6 years ago

  1. I think we could change self.env.config to self.config in each method of NotifyEmail by self.config = env.config in Notify.__init__().
  2. getlist() automatically call .strip() for each entry. I don't think x.strip() is needed.
    -        domains = self.env.config.getlist('notification', 'ignore_domains')
    -        self._ignore_domains = [x.strip().lower() for x in domains]
    +        domains = self.env.config.getlist('notification', 'ignore_domains')
    +        self._ignore_domains = [x.lower() for x in domains]
    
  3. Trivial thing: Currently smtp_always_cc and smtp_always_bcc can be separated by both space and comma characters. But getlist() use only one as separator. That behavior is undocumented in the options.

in reply to:  3 ; comment:4 by Ryan J Ollos, 6 years ago

Replying to jomae:

Revised changes in log:rjollos.git:t11636.1: addressing points (1) and (2) from comment:3, as well as some other issues.

  1. Trivial thing: Currently smtp_always_cc and smtp_always_bcc can be separated by both space and comma characters. But getlist() use only one as separator. That behavior is undocumented in the options.

Should we do anything to work around this? It could causes users trouble on upgrade if they have relied on using space separators. However, I'm not sure we want to continue to support unintended and inconsistent behavior, and the behavior of allowing space-separators seems to be a defect.

in reply to:  4 comment:5 by Jun Omae, 6 years ago

Replying to rjollos:

  1. Trivial thing: Currently smtp_always_cc and smtp_always_bcc can be separated by both space and comma characters. But getlist() use only one as separator. That behavior is undocumented in the options.

Should we do anything to work around this? It could causes users trouble on upgrade if they have relied on using space separators. However, I'm not sure we want to continue to support unintended and inconsistent behavior, and the behavior of allowing space-separators seems to be a defect.

The behavior might be intended. Cc field of ticket can be separated by space and comma characters, introduced in [1009]. I think smtp_always_{cc,bcc} was implemented to have the same behavior in [3231].

I think we could improve ListOption and getlist() to allow multiple separators. The improvement will help to keep the behavior. See jomae.git@t11636.2. Thoughts?

comment:6 by Ryan J Ollos, 6 years ago

Those changes look good. The properties that exist for backward compatibility could probably be marked for removal in a future release, such as 1.3.1.

comment:7 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Changes committed to trunk in [12848:12849].

comment:8 by Ryan J Ollos, 6 years ago

Improved wording of docstring in [12850].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.