Some very simple modifications #1

Merged
merged 3 commits into from Sep 11, 2014

Conversation

Projects
None yet
3 participants
Contributor

bserem commented Feb 16, 2014

Hello!

I saw your code and liked it a lot. I was looking for something like that.
I made some very small modifications, nothing trivial. Don't know if you like, but I'm creating the request anyway.

I'm also trying to communicate with the bot_jira maintainer in drupal.org to see if he plans to publish anything. If not I'll ask him to give me/us permissions to the module.

Bye,
Bill

@sirkitree sirkitree commented on the diff Feb 16, 2014

bot_jira.module
@@ -21,6 +22,21 @@ function bot_jira_menu() {
return $items;
}
+/**
+ * Implements hook_help().
+ */
+function bot_jira_help($path, $arg) {
+ switch ($path) {
+ case 'irc:features':
@sirkitree

sirkitree Feb 16, 2014

Owner

each case under a switch should be indented two more spaces (as well as what is within the case). Each case should also have a break; but since you're returning right from the case I think this is okay with out them.

@bserem

bserem Feb 16, 2014

Contributor

I shouldn't be trusting Vim's fix indent that much. Thanks for the heads up.

As for the breaks, I'll digg into it and see what is correct way.
I thought that return makes break unnecessary, but I've been wrong before :)

--- Original Message ---

From: "Jerad Bitner" notifications@github.com
Sent: February 16, 2014 4:31 AM
To: "sirkitree/bot_jira" bot_jira@noreply.github.com
Cc: "Bill Seremetis" bill@seremetis.net
Subject: Re: [bot_jira] Some very simple modifications (#1)

@@ -21,6 +22,21 @@ function bot_jira_menu() {
return $items;
}

+/**

  • * Implements hook_help().
  • */
    +function bot_jira_help($path, $arg) {
  • switch ($path) {
  • case 'irc:features':

each case under a switch should be indented two more spaces (as well as what is within the case). Each case should also have a break; but since you're returning right from the case I think this is okay with out them.


Reply to this email directly or view it on GitHub:
https://github.com/sirkitree/bot_jira/pull/1/files#r9775598

Owner

sirkitree commented Feb 16, 2014

Generally the code here is good and the feature is a good one. I tend to hardcode this because it avoids an extra database. The Drupal bot is already a big heavy for the purposes of a bot imho and this adds yet another call to an already slow bot, but this is also the standard way to do things in Drupal. So there is nothing really wrong with this technically, but I do wish there were a way to avoid the database call that the variable_get introduces. Thoughts?

Contributor

bserem commented Feb 16, 2014

Doesn't it get cached the first time and remain cached for some time?

I'll try to see what I can do with it tomorrow, my partner is more skilled in caching, we might come up with something.

Truth is that a bot on top of drupal doesn't seem to be the best idea, but it is dead easy to use/configure.
I will also evaluate egg drop some time later this month.

--- Original Message ---

From: "Jerad Bitner" notifications@github.com
Sent: February 16, 2014 4:36 AM
To: "sirkitree/bot_jira" bot_jira@noreply.github.com
Cc: "Bill Seremetis" bill@seremetis.net
Subject: Re: [bot_jira] Some very simple modifications (#1)

Generally the code here is good and the feature is a good one. I tend to hardcode this because it avoid an extra database. The Drupal bot is already a big heavy for the purposes of a bot imho and this adds yet another call to an already slow bot, but this is also the standard way to do things in Drupal. So there is nothing really wrong with this technically, but I do wish there were a way to avoid the database call that the variable_get introduces. Thoughts?


Reply to this email directly or view it on GitHub:
#1 (comment)

variable_get() data is always pre-loaded so calling it is never a database hit. The data is just always there and available.

@sirkitree sirkitree added a commit that referenced this pull request Sep 11, 2014

@sirkitree sirkitree Merge pull request #1 from bserem/7.x-1.x
Some very simple modifications
8adbfe0

@sirkitree sirkitree merged commit 8adbfe0 into sirkitree:master Sep 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment