-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements #9
base: master
Are you sure you want to change the base?
Improvements #9
Changes from 10 commits
8274bc3
17a3ea0
726d61a
c45e1e1
9f7bc88
dfbcc78
d409122
077a104
2ee35b3
9b83de9
ed5c9cb
e901b71
f4dbfe8
e3a0763
56c8b07
e64afd9
0596516
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"01" : | ||
{ | ||
"01" : [ "elvina.slovak" ] | ||
}, | ||
|
||
"02" : | ||
{ | ||
"02" : [ "john.doe", "jane.doe" ], | ||
"28" : [ "chris.nolan" ], | ||
}, | ||
|
||
"04" : | ||
{ | ||
"15" : [ "erwin.down", "leora.pontious", "randy.young" ] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
"01" : | ||
"01" : [ "elvina.slovak" ] | ||
|
||
"02" : | ||
"02" : [ "john.doe", "jane.doe" ] | ||
"28" : [ "chris.nolan" ] | ||
|
||
"04" : | ||
"15" : [ "erwin.down", "leora.pontious", "randy.young" ] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,26 +3,45 @@ | |
require 'birthday_reader' | ||
|
||
class BirthdayBot | ||
@@path = "birthdays.txt" | ||
|
||
def initialize() | ||
@config = ConfigReader.new | ||
@config.load('configurations.json') | ||
end | ||
|
||
def start! | ||
birthdays = BirthdayReader.get_birthdays(@@path) | ||
today = Time.now | ||
birthdays = BirthdayReader.get_birthdays(@config.birthdays_path) | ||
|
||
puts "Checking who was born today (#{today.to_s})" | ||
birthdays.each do |b| | ||
if (b[3].to_i == today.month) && (b[4].to_i == today.day) | ||
message = "#{@config.greeting_message} #{b[0]} #{b[1]}" | ||
HTTParty.post(@config.slack_url, body: { channel: @config.channel_name, | ||
username: @config.bot_name, | ||
text: message, | ||
icon_emoji: @config.bot_emoji }.to_json) | ||
puts "Checking who was born today ( #{ Time.now.to_s } )" | ||
unless birthdays.nil? | ||
users = "#{ mention birthdays[0] }" | ||
if birthdays.count > 1 | ||
puts "#{ birthdays.count } people were born today" | ||
if birthdays.count == 2 | ||
users = " #{ mention birthdays[0] } and #{ mention birthdays[1] } " | ||
else | ||
for i in 1..birthdays.count-2 | ||
users.concat( ", #{ mention birthdays[i] }" ) | ||
end | ||
users.concat( " and #{ mention birthdays[i+1] } " ) | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can simplify this:
What do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shinenelson Did you take a look at this ? (mentions are missing here) |
||
message = "#{users} #{@config.greeting_message}" | ||
HTTParty.post(@config.slack_url, body: { channel: @config.channel_name, | ||
username: @config.bot_name, | ||
text: message, | ||
icon_emoji: @config.bot_emoji }.to_json) | ||
else | ||
puts "Today is a day that no one was born" | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you looked at my previous comment with a suggestion to refactor this function ?
|
||
end | ||
|
||
def mention ( name ) | ||
if @config.mention | ||
"<@#{ name }>" | ||
else | ||
name | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you give an example where disabling mentions is useful ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The earlier version of the database stored the real names of users. The bot wished users with their real names. So, I thought that could be a valid use-case But like my commit message said, some people might prefer wishing their team-mates by their real names (probably for more family-togetherness) and/or not choosing to spam the users with mentions on the wishes (but that wil probably be taken care of by other team-mates, anyway). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, thanks ! |
||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,13 @@ | ||
require 'yaml' | ||
|
||
class BirthdayReader | ||
def self.get_birthdays(file_path) | ||
birthdays = [] | ||
if File.exist?(file_path) | ||
file = File.new(file_path, 'r') | ||
file.each_line do |line| | ||
birthdays << line.chomp.split | ||
end | ||
file.close | ||
bday_list = YAML.load_file(file_path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of this class is to read birthdays only, I don't think we should return only the birthdays according to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The primary reason I switched to a structured format for the database was to drill down to the exact date to fetch all the birthdays on the day. What would be the use case to have return all the birthdays together? It would just use more memory (for no particular gain, whatsoever) IMHO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not against the structuring. What I am saying is that the purpose of the function is to read birthdays. It is not to read and filter. That is why I think we should not filter the birthdays here. And because we are just managing strings of birthdays I guess memory is not a concern. |
||
unless bday_list.nil? || bday_list.empty? | ||
birthdays = bday_list[Time.now.month.to_s][Time.now.day.to_s] | ||
end | ||
end | ||
return birthdays | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
puts "Checking who was born today (#{ Time.now.to_s })"
remove the spaces.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt it would be more legible with the spaces.