Improvements #9

shinenelson · 2017-06-04T21:05:00Z

This is almost like a whole re-factor of the bot.
The commit messages should be self-explanatory.

Change birthdays database to YAML / JSON
The YAML module can load and parse JSON files, so that's an added benefit. I initially thought of using only JSON since the configuration file was JSON too. But since YAML is a more easier-to-understand format for everyone, so I thought I'd add support for that as well.
Configurize db_path and mention
- The birthdays.txt was being hard-coded and loaded from the code. This makes the bot less-flexible in terms of the database filename. Since I was re-factoring the code, I thought why not. Configurizing the database filename would make it more flexible. But the database file should still be parse-able by the YAML module, so that's there.
- Some people prefer wishing their team members with real names while others like mentioning them and spam them with mentions all day. Mentioning is configurable too. This too depends on what values are loaded from the database. If the database has real names and the mention flag is true, the purpose is defeated.
Renaming configurations.json to configurations.json.example - because that's what it is, only an example placeholder file, not an actual configuration file.
The bot has been optimized to work with the above changes and the README file has been updated with more information with regard on how to configure the bot and use it.

Parsing plain text files requires more processing than structured file types like YAML / JSON. YAML support is built-into ruby > 1.9.3. The YAML module can easily load a JSON file too. Why was the year field removed? Birthdays are recurring. You need to wish people every year. Why would you want to torment people with the notion of them getting old? 😜

because that is what it is - an example / placeholder configuration file

Supports multiple birthdays from arrays. Earlier version sent multiple wishes for multiple birthdays, this version appends the users to a single array and sends out a single birthday wish message.

You can now configure the birthday database with your team memebers' usernames or real names. The bot will mention the users based on this parameter.

mmachado95 · 2017-06-04T21:06:53Z

@teresalazar13

jbernardo95

@shinenelson Awesome PR ! :) The birthday settings with json/yaml is a really nice addition.

Left a couple of comments for you to take a loo.

jbernardo95 · 2017-06-04T21:26:49Z

lib/birthday_reader.rb

-        birthdays << line.chomp.split
-      end
-      file.close
+	bday_list = YAML.load_file(file_path)


The purpose of this class is to read birthdays only, I don't think we should return only the birthdays according to Time.now, instead we should return all of them.

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.

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.

jbernardo95 · 2017-06-04T21:28:55Z

configurations.json

@@ -1,4 +1,5 @@
 {
+  "db_path":	      "birthdays.yaml",


I think it makes more sense to change db_path to birthdays_path

jbernardo95 · 2017-06-04T21:35:48Z

lib/birthday_bot.rb

-                                                 username: @config.bot_name,
-                                                 text: message,
-                                                 icon_emoji: @config.bot_emoji }.to_json)
+    puts "Checking who was born today"


I think we should keep Time.now in the log

Ah, you wanted it in the log. The way I handle my crons are, I prepend date before my executing command. But I don't know if this works with Heroku's scheduler (does it?). Else I'll just put the date back in the log.

jbernardo95 · 2017-06-04T22:02:09Z

lib/birthday_bot.rb

+             users += ", <@#{ birthdays[i] }>"
+           end
+           users += " and <@#{ birthdays[i+1] }> "
+        end
      end


I think we can simplify this:

puts "#{birthdays.count} were born today" today = Time.now birthdays_today = birthdays[today.month][today.day] unless birthdays_today.empty? users = build_user_list(birthdays_today) 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) end # ... def build_user_list(birthdays) if birthdays.size == 1 birthdays.first else users = birthdays.take(birthdays.count - 1).join(", ") users += " and #{birthdays[birthdays.count - 1]}" if birthdays.count > 1 end end

What do you think ?

@shinenelson Did you take a look at this ?

(mentions are missing here)

jbernardo95 · 2017-06-04T22:10:15Z

lib/birthday_bot.rb

+    else
+      name
+    end
+  end


Can you give an example where disabling mentions is useful ?

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).

Makes sense, thanks !

The YAML module parses keys with a leading 0 as octcal digits, invalidating 08 and 09. Parsing them as strings would be safer

shinenelson · 2017-08-01T18:03:59Z

I'll leave a friendly reminder here that this pull request is still un-merged.

Also, I've handled 2 new issues that I came across.

YAML parses values with leading 0 as octal digits, it would be safer if we parsed the dates as strings than just integers.
Handled an exception with empty array after YAML.load.

mmachado95 · 2017-08-01T23:25:03Z

@jbernardo95

jbernardo95

@shinenelson Sorry for the late reply I didn't get any notifications with updates on this.

I left a few extra comments for you to take a look.

Next time tag me using @jbernardo95 to make sure I see the notifications.

jbernardo95 · 2017-08-02T20:46:18Z

lib/birthday_reader.rb

-        birthdays << line.chomp.split
-      end
-      file.close
+	bday_list = YAML.load_file(file_path)


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.

jbernardo95 · 2017-08-02T20:48:08Z

lib/birthday_bot.rb

-                                                 username: @config.bot_name,
-                                                 text: message,
-                                                 icon_emoji: @config.bot_emoji }.to_json)
+    puts "Checking who was born today ( #{ Time.now.to_s } )"


puts "Checking who was born today (#{ Time.now.to_s })" remove the spaces.

I felt it would be more legible with the spaces.

jbernardo95 · 2017-08-02T20:49:59Z

lib/birthday_bot.rb

+                                               text: message,
+                                               icon_emoji: @config.bot_emoji }.to_json)
+    else
+      puts "Today is a day that no one was born"
    end


Have you looked at my previous comment with a suggestion to refactor this function ?

puts "#{birthdays.count} were born today" today = Time.now birthdays_today = birthdays[today.month][today.day] unless birthdays_today.empty? users = build_user_list(birthdays_today) 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) end # ... def build_user_list(birthdays) if birthdays.size == 1 birthdays.first else users = birthdays.take(birthdays.count - 1).join(", ") users += " and #{birthdays[birthdays.count - 1]}" if birthdays.count > 1 end end

shinenelson · 2019-11-21T14:07:52Z

I don't know whether this repository is still in maintenance, but I had a use-case for this bot that made me come back to it. Upon navigating the repository is when I remembered that I hadn't worked on the changes that @jbernardo95 requested here. I've now made the changes that were requested.

Plus, I refactored away the HTTParty gem dependency that was kind of redundant because ruby already has the net/http standard library built-in. Besides, it was the only dependency and since there's only one outbound HTTP request, it wasn't too much of work to modify the request to work with the net/http standard library.
Of course, it's extra lines of code, but I think it's worth it so that we can remove one redundant dependency ( and the Gemfile altogether ).

HTTParty is the only gem dependency of the bot. I thought it was redundant to maintain a single dependency especially when it was achievable with the standard net/http library except that it would some extra lines of code. I thought the extra lines of code was worth it rather than having to maintain a single dependency.

shinenelson added 6 commits June 5, 2017 01:23

Configurize db_path for the birthdays database

17a3ea0

Rename configurations.json to configuration.json.example

726d61a

because that is what it is - an example / placeholder configuration file

Optimize bot to work with {hash}ed birthdays

c45e1e1

Supports multiple birthdays from arrays. Earlier version sent multiple wishes for multiple birthdays, this version appends the users to a single array and sends out a single birthday wish message.

Configurize mention'ing team members

9f7bc88

You can now configure the birthday database with your team memebers' usernames or real names. The bot will mention the users based on this parameter.

Update README.md with more detailed instructions

dfbcc78

jbernardo95 requested changes Jun 4, 2017

View reviewed changes

shinenelson added 4 commits June 5, 2017 04:26

Rename db_path to birthdays_path (Resolves jeKnowledge#9 (comment))

d409122

Log Time.now

077a104

change date keys to string

2ee35b3

The YAML module parses keys with a leading 0 as octcal digits, invalidating 08 and 09. Parsing them as strings would be safer

handle empty / nil YAML.load_file

9b83de9

jbernardo95 requested changes Aug 2, 2017

View reviewed changes

shinenelson added 4 commits August 3, 2017 22:47

remove spaces (Resolves jeKnowledge#9 (comment))

ed5c9cb

move birthday list filter to birthday_bot from birthday_reader

e901b71

add today = Time.now (Resolves jeKnowledge#9 (comment))

f4dbfe8

def build_user_list

e3a0763

shinenelson force-pushed the improvements branch from ab417b8 to b282753 Compare November 21, 2019 13:28

shinenelson added 3 commits November 22, 2019 02:12

[docs] replace db_path with birthdays_path

e64afd9

[readme] fix URL in contributors' list

0596516

shinenelson force-pushed the improvements branch from b282753 to 0596516 Compare November 21, 2019 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements #9

Improvements #9

shinenelson commented Jun 4, 2017

mmachado95 commented Jun 4, 2017

jbernardo95 left a comment

jbernardo95 Jun 4, 2017

shinenelson Jun 4, 2017

jbernardo95 Aug 2, 2017

jbernardo95 Jun 4, 2017

jbernardo95 Jun 4, 2017

shinenelson Jun 4, 2017

jbernardo95 Jun 4, 2017

jbernardo95 Jun 5, 2017

jbernardo95 Jun 4, 2017

shinenelson Jun 4, 2017

jbernardo95 Jun 5, 2017

shinenelson commented Aug 1, 2017

mmachado95 commented Aug 1, 2017

jbernardo95 left a comment •

edited

Loading

jbernardo95 Aug 2, 2017

jbernardo95 Aug 2, 2017

shinenelson Aug 3, 2017

jbernardo95 Aug 2, 2017

shinenelson commented Nov 21, 2019

Improvements #9

Are you sure you want to change the base?

Improvements #9

Conversation

shinenelson commented Jun 4, 2017

mmachado95 commented Jun 4, 2017

jbernardo95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinenelson commented Aug 1, 2017

mmachado95 commented Aug 1, 2017

jbernardo95 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinenelson commented Nov 21, 2019

jbernardo95 left a comment •

edited

Loading