Skip to content
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

Extensive additions and refactoring #15

Closed
wants to merge 53 commits into from

Conversation

cgarrigues
Copy link

I have made considerable changes to the code including extensive refactoring. Suggest you look at my README file to see if you want my changes.

1) Put all Tickets I’m watching in OmniFocus instead of just the ones
that were assigned to me
2) Flag the tasks if the ticket is assigned to me; unplug it if it isn’t
3) Set context based on who submitted the ticket
use of keychain doesn’t work under cron, so we’re doing it the right
way now.
Also don’t bomb out if assignee is blank
whoops, lost the duedate field
instead of puts to report information.
@cgarrigues cgarrigues changed the title Master Extensive additions and refactoring Sep 28, 2015
@devondragon
Copy link
Owner

@cgarrigues,

There are some changes I would love to merge in (launchd instead of cron, keychain instead of plaintext password, etc...). However the custom field parent task stuff, and the reporter->context name thing, growl dependency, etc.. are all essentially breaking changes for my use case. I'm also not convinced that the jira-ruby module is the way to go as it seems to have a lot of open issues and very little active development/support, where as the simple http/json stuff is easily maintained by me.

I'm pretty new to github, so I'm not sure what the best way to handle this PR is...

Thoughts?

@cgarrigues
Copy link
Author

I'm also new to github, sorry to say. Between us maybe we can figure it out.

I did a lot of refactoring to get where I am, so it's not obvious just how to get to where both of us would be happy with the code.

I'm thinking that working on my branch to make some of the new dependencies (growl, for example) optional so that it can handle your use case again might be easier than figuring out how to apply the pieces of mine that you like against your code since some of the refactoring was in order to get them to work easily. (some, like launchd, will be easy, others much less so.)

FWIW, I'm also fairly new at ruby, but after N decades of coding, I have a habit of deep diving into new technologies to figure out what's considered the "right" way to do things with my new technologies.

@devondragon
Copy link
Owner

Maybe it makes sense to you keep your fork, for your use case, and if it's okay with you, I will work on manually merging in the commonly desired changes. It leaves your fork a little bit detached, but might be easiest path forward...

@cgarrigues
Copy link
Author

cgarrigues commented Oct 7, 2015 via email

@devondragon
Copy link
Owner

Haha I am:) Quick question, did you have any issues getting the Keychain stuff working? I'm getting nil back instead of a usable keychain item. No errors.

cgarrigues added 3 commits November 12, 2015 09:29
At the moment, it won’t call growl at all.  Future patch will enable
growl if it’s installed, but at the moment, it’s Notification Center
only.
@devondragon
Copy link
Owner

Manual feature merging has been done. I have been unable to get keychain support working correctly, but the rest of it is functioning as expected. Closing this PR.

@cgarrigues
Copy link
Author

My version has moved on slightly since then. I had to use a loaner mac for a short while and that gave me the opportunity to try it w/o the environment fully configured.

  • better errors if stuff isn’t set up yet (including keychain). This includes an error that tells you what to do if your keychain isn’t already set up. For some reason mine was already there when I set it up originally. I had to figure out how to set it up by hand on my loaner mac. This is probably your issue.
  • if Growl isn’t installed, fall back to Notification Center.

Chris

On Nov 16, 2015, at 1:49 PM, Devon Hillard <[email protected]mailto:[email protected]> wrote:

Manual feature merging has been done. I have been unable to get keychain support working correctly, but the rest of it is functioning as expected. Closing this PR.


Reply to this email directly or view it on GitHubhttps://github.com//pull/15#issuecomment-157149572.

@devondragon
Copy link
Owner

Chris,

Great! The new error message totally sorted things out for me. Manually merged. All good!

@cgarrigues
Copy link
Author

Excellent. I’m still not clear why mine was already configured on the other system, but now we know how to make it work for anybody.

When I’ve got time I’ll see if I can get your version and mine closer to each other. I’d like to get to identical code bases at some point.

On Nov 19, 2015, at 6:18 PM, Devon Hillard <[email protected]mailto:[email protected]> wrote:

Chris,

Great! The new error message totally sorted things out for me. Manually merged. All good!


Reply to this email directly or view it on GitHubhttps://github.com//pull/15#issuecomment-158241301.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants