Refactoring of trac.notification

[Ryan J Ollos]

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

[Jun Omae]

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.

[Ryan J Ollos]

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.

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.

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.

[Jun Omae]

Replying to rjollos:

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.

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?

[Ryan J Ollos]

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.

[Ryan J Ollos]

Changes committed to trunk in [12848:12849].

[Ryan J Ollos]

Improved wording of docstring in [12850].