[PATCH] notify.py: Use a/an correctly when sending request notifications
Will no longer send notifications about "a orphan request", but determine whether to use a/an based on the first character of the request type. Signed-off-by: Lars Rustand <rustand.lars@gmail.com> --- aurweb/scripts/notify.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py index d975086..ace6614 100755 --- a/aurweb/scripts/notify.py +++ b/aurweb/scripts/notify.py @@ -414,8 +414,9 @@ class RequestOpenNotification(Notification): (self._user, self._pkgbase, self._merge_into) body += '\n\n' + self._text else: - body = '%s [1] filed a %s request for %s [2]:' % \ - (self._user, self._reqtype, self._pkgbase) + an = ["a","an"][self._reqtype[0] in "aeiou"] + body = '%s [1] filed %s %s request for %s [2]:' % \ + (self._user, an, self._reqtype, self._pkgbase) body += '\n\n' + self._text return body -- 2.22.0
On 8/9/19 12:37 PM, Lars Rustand wrote:
Will no longer send notifications about "a orphan request", but determine whether to use a/an based on the first character of the request type.
Thanks, looks like a reasonable change. See below for implementation nitpicks.
Signed-off-by: Lars Rustand <rustand.lars@gmail.com> --- aurweb/scripts/notify.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py index d975086..ace6614 100755 --- a/aurweb/scripts/notify.py +++ b/aurweb/scripts/notify.py @@ -414,8 +414,9 @@ class RequestOpenNotification(Notification): (self._user, self._pkgbase, self._merge_into) body += '\n\n' + self._text else: - body = '%s [1] filed a %s request for %s [2]:' % \ - (self._user, self._reqtype, self._pkgbase) + an = ["a","an"][self._reqtype[0] in "aeiou"]
Using ['a', 'an'][True] as a list index by relying on it equaling the `1` offset due to implicitly `int(True)` equaling `1` feels pretty unreadable and I don't think I've ever seen that pattern, what about instead using a standard ternary operator: an = 'an' if self._reqtype[0] in 'aeiou' else 'a' Also: the aurweb codebase generally uses single quotes (and does, for the surrounding lines), so I think we should stick to that.
+ body = '%s [1] filed %s %s request for %s [2]:' % \ + (self._user, an, self._reqtype, self._pkgbase) body += '\n\n' + self._text return body
-- Eli Schwartz Bug Wrangler and Trusted User
Will no longer send notifications about "a orphan request", but determine whether to use a/an based on the first character of the request type. Signed-off-by: Lars Rustand <rustand.lars@gmail.com> --- aurweb/scripts/notify.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py index d975086..591b5ca 100755 --- a/aurweb/scripts/notify.py +++ b/aurweb/scripts/notify.py @@ -414,8 +414,9 @@ class RequestOpenNotification(Notification): (self._user, self._pkgbase, self._merge_into) body += '\n\n' + self._text else: - body = '%s [1] filed a %s request for %s [2]:' % \ - (self._user, self._reqtype, self._pkgbase) + an = 'an' if self._reqtype[0] in 'aeiou' else 'a' + body = '%s [1] filed %s %s request for %s [2]:' % \ + (self._user, an, self._reqtype, self._pkgbase) body += '\n\n' + self._text return body -- 2.22.0
On Fri, 09 Aug 2019 at 13:47:18, Lars Rustand wrote:
Will no longer send notifications about "a orphan request", but determine whether to use a/an based on the first character of the request type.
Signed-off-by: Lars Rustand <rustand.lars@gmail.com> --- aurweb/scripts/notify.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/aurweb/scripts/notify.py b/aurweb/scripts/notify.py index d975086..591b5ca 100755 --- a/aurweb/scripts/notify.py +++ b/aurweb/scripts/notify.py @@ -414,8 +414,9 @@ class RequestOpenNotification(Notification): (self._user, self._pkgbase, self._merge_into) body += '\n\n' + self._text else: - body = '%s [1] filed a %s request for %s [2]:' % \ - (self._user, self._reqtype, self._pkgbase) + an = 'an' if self._reqtype[0] in 'aeiou' else 'a'
Good catch! Thanks for the patch. Note that I am not the biggest fan of implementing this logic using a rule of thumb that is hidden somewhere in the code base and does not always yield correct results (fortunately, we don't have "hourly requests" yet). That being said, the simple dictionary based solution { 'delete': 'a', 'merge': 'a', 'orphan': 'an' }[reqtype] I had in mind is probably not a good idea either: the notification module is entirely independent from the supported request types and does not hardcode any of these values anywhere else. I will merge this patch as a workaround. We should really fix the notification system and come up with a proper solution, though.
+ body = '%s [1] filed %s %s request for %s [2]:' % \ + (self._user, an, self._reqtype, self._pkgbase) body += '\n\n' + self._text return body
-- 2.22.0
participants (3)
-
Eli Schwartz
-
Lars Rustand
-
Lukas Fleischer