Refactor renderers

[ttaylorr]

This pull-request is a bit lengthy, but isn't actually too bad.

Things refactored:

• Various, small refactors of the pattern_generator class
• Move each pattern generator into its own class
• Update the generate_pattern method to use these new classes
• Remove some duplicated code in the pattern classes (specifically, tessellation_pattern.rb and plaid_pattern.rb

This also bumps the version to 1.3.0.

[jasonlong]

Nice. Thanks, @ttaylorr. I'll pull this down and test it out when I get chance (hopefully this week, but I'll be traveling for Thanksgiving a bit).

Since this is all cleanup and refactoring (and no new functionality), let's go with bumping the version to 1.2.2.

[ttaylorr]

Sure, that sounds 🆒 to me. Still a few things left before this is ready to be tested. I'll remove the "[WIP]" from the title once we're good to go.

[ttaylorr]

Did a smoke test locally and everything seems to be 👍. Feel free to take a look yourself and merge away.

/to @jasonlong

[jasonlong]

@ttaylorr I didn't notice until just now that this breaks the current syntax for specifying a pattern.

:generator => "sine_waves"

vs.

:generator => GeoPattern::SineWavePattern

I definitely want to keep this backwards compatible with what's in place now. I didn't dive too deeply into your restructuring, but I fear this might be a big monkey wrench. 😬

[ttaylorr]

Yeah, this is why I initially was going for a version bump to 1.3.0. I figure we can just map between the string and its class counterpart?

To be fair though, I think breaking that old behavior might actually be a win here. Seems much clearer to me that we pass class names instead of strings to opts.

[jasonlong]

TBH I prefer the simplicity of a string like sine_waves to the verbosity of GeoPattern::SineWavePattern, but I'd love to get more input from others on this.

[mdarby]

IMHO I prefer the :generator => GeoPattern::SineWavePattern pattern. It's more verbose yes, but it also tells me exactly what class is at play. sine_waves tells me the output, but not where to go to debug it.

[tarebyte]

Just for my OCD you would mind changing the line to look like this?

<div style="background-image: <%= pattern.uri_image %>"></div>

[jasonlong]

👍

[btmills]

The interface @ttaylorr proposed would work well from the perspective of the JS port.

GeoPattern.generate('GitHub', { generator: GeoPattern.SineWavePattern });

• As @mdarby pointed out, while it's slightly more verbose, it's also more transparent.
• If I were to move all of the generators to a Geopattern.patterns object, it would be simple to enumerate available patterns, say in the browser JS console, without having to reference the docs.
• I can get tab completion of patterns in the JS console.

In general I think the proposed interface does a good job of improving discoverability compared to using strings.

If I were to do this in the JS port, I'd do a minor version bump to 1.3 due to the new API, but I'd maintain backwards compatibility until a hypothetical 2.0 by having the generate method also accept the existing string names and map them to the generators.

[jasonlong]

Well alrighty, you guys have convinced me. Let's do this. 🚀

I agree that this warrants a bump to 1.3.0. @ttaylorr, would you be able add support for the current string syntax and possibly a deprecation warning for those still using that syntax?

[ttaylorr]

Glad you agree, @jasonlong - thanks for having others weight in on this. 👍

I'll bump back to 1.3.0, and allow a mapping of Strings to classnames (the PATTERNS array becomes a hash) and puts a small warning if a string is provided.

Thanks for the feedback, everybody!

[ttaylorr]

[jasonlong]

Thanks @ttaylorr. I'll kick the tires some more shortly and hopefully get this merged.

[jasonlong]

Using puts here includes this in the generated svg, making it invalid. I think if we just output it like a comment, we'd be 👌. Like <!-- String pattern references are deprecated as of 1.3.0 -->.

[jasonlong]

Sorry @ttaylorr, I've been pretty busy and didn't see you made that change already (which looks good btw). I was testing this out and noticed a problem. I'll add a comment below.

[jasonlong]

When I use the new :generator => GeoPattern:DiamondsPattern syntax, this always evaluates as false. Strings seem to be working fine. I can investigate, but I need to run out for a bit. If you have time, maybe you could take a peek.

[jasonlong]

Hey @ttaylorr, this check still never passes for me when opts[:generator] is something like GeoPattern::ConcentricCirclesPattern. Is it working for you? Wondering if it's a difference w/ Ruby versions.

[ttaylorr]

I've forgotten how to Ruby. 😞

[jasonlong]

💥 That's better.

Also, I was thinking we should be consistent with the plural naming convention. For example, the strings are all plural (e.g. chevrons) and the class names are sometimes plural (DiamondsPattern), but usually not (SquarePattern). Do you have a preference for the new convention? I'm fine as long as they're consistent.

[ttaylorr]

I think we should opt for the singular form. However, I think patterns with an adjective preceeding the shape should remain as-is: i.e., MosiacSquaresPattern

[jasonlong]

[ttaylorr]

🤘 🚀 🚢