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

Add Support for Plugins #64

Merged
merged 8 commits into from Jul 29, 2012

Conversation

Projects
None yet
3 participants
@kenmazaika
Copy link
Contributor

kenmazaika commented Jul 22, 2012

This is a version of #63, that merges cleanly and has all tests passing.

Also with a few small changes:

  • fix broken tests
  • add cucumber test for setting / showing plugin configuration
  • add cucumber test for the dot_com plugin (which is disabled by default)

@kenmazaika kenmazaika referenced this pull request Jul 22, 2012

Closed

Add Support For Plugins #63

@mroth

This comment has been minimized.

Copy link
Owner

mroth commented Jul 23, 2012

This is really, really awesome. I've started reviewing the code (I want to make sure I understand it completely) but it will probably take me a while to get through it all. I appreciate all the hard work you've done on this, bear with me as I'll likely have some dumb questions.

@mroth

This comment has been minimized.

Copy link
Owner

mroth commented Jul 23, 2012

Minor bug/regression: when using lolcommits --test --capture should work regardless of whether you are currently in a git repository, and the test images should be stored in a "test" subdirectory of the lolimages rather than along with a repository if you are currently in one. I just wrote two integration tests for these scenarios that pass on master and illustrate the bugs, I'll push those to master and you can rebase/merge them into your branch to help debug.

mroth added a commit that referenced this pull request Jul 23, 2012

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Jul 23, 2012

Good catch, I hadn't noticed that. I'll fix this in the next few days. Thanks for writing the test to catch the issue.

class Class
def subclasses
result = []
ObjectSpace.each_object(Class) { |klass| result << klass if klass < self }

This comment has been minimized.

@mroth

mroth Jul 23, 2012

Owner

This is cool, I wasn't aware of this pattern before.

This comment has been minimized.

@kenmazaika

kenmazaika Jul 24, 2012

Author Contributor

I'd like to take credit, but I lifted it from StackOverflow: http://stackoverflow.com/questions/436159/how-to-get-all-subclasses

require "choice"
require "fileutils"
require "git"
require "launchy"
require "RMagick"

This comment has been minimized.

@mroth

mroth Jul 24, 2012

Owner

With the new plugin structure, would it make sense to move requirements like this into the plugin itself for greater modularity? i.e. since now RMagick is only used with the loltext plugin it could be required in /plugins/loltext.rb ?

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Jul 24, 2012

Great feedback.

  • I adjusted the testmode to always use the 'test' loldir, and shuffled around the configuration stuff a little.
  • I shuffled around some of the requires in lolcommits.rb
  • I added support for changing the configuration of the test mode with the following commands
lolcommits --test --configure
lolcommits --test --show-config
@mroth

This comment has been minimized.

Copy link
Owner

mroth commented Jul 24, 2012

Playing with this more, very cool.

I'm trying to figure out the raw images being stored stuff. Is this a strong requirement for the lolcommits server as you have it implemented? I'm concerned about the amount this increases the storage footprint, especially since the source images are in many cases significantly larger (megapixel+) as webcam resolution increases.

If it's required, I think we should probably abstract out the raw images storage into a plugin of it's own, that can run prior to the loltext plugin running -- it could copy the tmp_snapshot (which should be deleted after loltext runs) to a permanent raw.sha.jpg in a raw subdirectory of the loldir for the repo, and possibly resize it to a standardized size? (Alternately, if it's not required, we could just drop it for now).

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Jul 24, 2012

It's not a hard requirement, but I think it's a useful feature to have.

I like the idea of storing the raw file as a temp file that gets cleaned up after the process runs. I'll work on getting that working.

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Jul 25, 2012

I just made a few changes, so I think this does what we want.

  • For the raw file always use the same static file name (tmp_snapshot.jpg, what it was before).
  • After capture resize the raw image to at most 640x480. This means whether loltext is enabled or not, a resized image will be saved.
  • After resize, set that as the default main image initially (so if no plugins run, we will have a main image).
  • When loltext runs, it no longer will resize, but it will add the text, and replace the existing main image.
  • After all the plugins run, a cleanup! function will remove the raw file.

I decided not to use a plugin for this functionality, because we didn't need to configure the behavior specially for different users, and it seems reasonable to do this behavior for any configuration.

Let me know if you see anything that you think should be changed.

@mroth

This comment has been minimized.

Copy link
Owner

mroth commented Jul 29, 2012

I did a merge into my refactor branch to resolve the merge conflicts, and then did some work on this to fix a few minor regressions, and also found and fixed some other problems with my codebase at the same time. You can see my changes at #66, which I'm about to merge into master as 0.3.0pre1

Thanks so much for all your help!

@mroth mroth merged commit 4173317 into mroth:master Jul 29, 2012

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Jul 30, 2012

Very cool. I'm excited the code got merged into master. Thanks for working with me, and identifying / fixing the regressions that the my code added.

I'm interested to see what features get merged in down the road.

@matthutchinson

This comment has been minimized.

Copy link
Collaborator

matthutchinson commented Feb 13, 2018

Hi @kenmazaika 👋

Wow, 2012, simpler times they were... the plugins have come along way since this first commit

So you may (or may not) have noticed that we recently extracted all lolcommit plugins to external gems!

🏅 Since you contributed this dotcom plugin, I've added you (as an admin) to the new lolcommits-dotcom and lolcommits-tranzlate repos.

Of course you are under no obligation to maintain this code or gem going forward. This is just a quick message to explain what has happened and give you the opportunity to take ownership again.

If you're interested in picking this up, send me your email address (to matt/at/hiddeloop.com) and I'll add you as a new gem owner (with publishing rights) here and here.

Otherwise I will continue to maintain the gem, and make sure things are kept up to date.


To get an idea of how gem plugins work, check out the sample_plugin README / RubyDocs or browse through other plugin code.

The main reasons for extraction were to:

  • remove the ever-growing gem dependency list in the main lolcommits gem
  • start to unify configuration code for plugins (common helpers, approaches to OAuth etc.)
  • make lolcommit plugin development easier
  • add more comprehensive plugin tests

That's it, any questions just let me know.

Happy coding!

Matt

@kenmazaika

This comment has been minimized.

Copy link
Contributor Author

kenmazaika commented Feb 13, 2018

Thanks, @matthutchinson! Awesome to see the great progress on lolcommits and the different plugins that are now available!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.