Closed ttaylorr closed 9 years ago
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
.
Sure, that sounds :cool: 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.
Did a smoke test locally and everything seems to be :+1:. Feel free to take a look yourself and merge away.
/to @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. :grimacing:
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
.
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.
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.
The interface @ttaylorr proposed would work well from the perspective of the JS port.
GeoPattern.generate('GitHub', { generator: GeoPattern.SineWavePattern });
Geopattern.patterns
object, it would be simple to enumerate available patterns, say in the browser JS console, without having to reference the docs.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.
Well alrighty, you guys have convinced me. Let's do this. :rocket:
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?
Glad you agree, @jasonlong - thanks for having others weight in on this. :+1:
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! :octocat:
:shipit:
Thanks @ttaylorr. I'll kick the tires some more shortly and hopefully get this merged.
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.
I've forgotten how to Ruby. :disappointed:
:boom: 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.
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
:metal: :rocket: :ship:
This pull-request is a bit lengthy, but isn't actually too bad.
Things refactored:
pattern_generator
classtessellation_pattern.rb
andplaid_pattern.rb
This also bumps the version to
1.3.0
.