Closed AndrewRadev closed 9 years ago
@AndrewRadev I honestly hadn't considered this, but I'm not opposed to it. Our security team at GitHub wanted the PATTERNS
check in place, so I'll need to keep them in the loop if we move forward with this.
I'm assuming this would involve putting the geo_my_pattern
methods in an external Ruby file, correct? Feel free to write your thoughts on how best to implement this and then we can start gathering feedback. Thanks!
To implement something like this in the JavaScript version, I'd probably overload options.generator to accept either a string (the name of a pattern) or a function, like this example (pulled from the sine waves pattern):
GeoPattern.generate('GitHub', { generator: function (hash, svg, helpers) {
var period = Math.floor(helpers.map(helpers.hexVal(hash, 0), 0, 15, 100, 400));
// ...
svg.setWidth(period);
// ...
for (i = 0; i < 36; i++) {
val = helpers.hexVal(hash, i);
opacity = helpers.fillOpacity(val);
fill = helpers.fillColor(val);
xOffset = period / 4 * 0.7;
// ...
svg.path(str, styles).transform({
translate: [
-period / 4,
waveWidth * i - amplitude * 1.5
]
});
}
// ...
}});
The function signature is something I just threw together quickly, but the goal is to make available anything the generator might need, so the helpers
argument includes map
, hexVal
, and other useful functions.
My Ruby is weak, but I think the equivalent might be something like the following:
GeoPattern.generate("Mastering Markdown", { :generator => Proc.new {|svg, helpers| ... } })
Is that something that's even remotely idiomatic and viable, and does it accomplish what you're trying to do @AndrewRadev?
@btmills My original idea was something more along the lines of:
GeoPattern.define_pattern('foo') do |svg, helpers, etc|
# ...
end
But your suggestion is very nice as well and has the benefit that you don't have to do a separate definition. The example is reasonably idiomatic and has the benefit that any ruby callable can be put in place of the proc. I assume most people would do something like:
class CustomPatternGenerator
def call(svg, *etc)
end
end
GeoPattern.generate("Foo", generator: CustomPatternGenerator.new)
(An object with the call
method is a standard way of imitating a proc in ruby)
With this, it won't be necessary to change the PATTERNS
array logic, since we could just check if "generator" is callable instead. Not sure what the github security team think about this, since I'm not sure what the reasoning behind PATTERNS
is either. @jasonlong should probably ask about this.
I think the biggest problem will be figuring out a good set of helpers to provide to the pattern generator. It has to be something that provides a stable, preferably well-documented API. The list of hash
, svg
, helpers
that @btmills proposes might be good enough, but I can't say for sure right now, I'd need some more poking around. I currently have a pretty hacky fork that works for me, but with some massaging. For instance, the group
helper is very useful, but I'd like to be able to use it with a block:
svg.group(transform: 'rotate(45)') do
svg.rect('....')
# ....
end
The existing usage of group
works quite well as it is, but for extending, a more flexible API will be beneficial. This is why I think it'll take a little bit of effort and discussion to get to a nice set of helpers. Hopefully, not a whole lot :). I'll try to familiarize myself with the codebase and make some suggestions soonish, though if anyone else has a good idea of the API and is willing to put some work in it, I wouldn't turn them down :).
This is pretty stale, so I'm going to close for now. Feel free to reopen if there's new thoughts.
I'd like to be able to create a custom pattern. Are there any plans to provide some way to "register" a pattern for the gem to use? Right now, if you just monkeypatch the
Pattern
class to add ageo_my_pattern
method, you get an exception due tomy_pattern
not being present in thePATTERNS
list. It's still possible to hack this up, but I'd prefer a more stable way to customize.I could work on and propose some form of API in a pull request, but I figured it's better to ask first.