Make it possible to reduce available patterns #40

maxmeyer · 2015-01-07T22:09:48Z

This PR is based on rspec-PR and testsuite-PR.

This PR makes it possible to reduce the available patterns. To make this possible it moves code from the PatternGenerator-class to a separate class I named PatternDb. This class handles all the checks etc. To make the library more testable I also moved some code to helpers methods I can also use in the tests.

maxmeyer · 2015-01-07T22:18:29Z

I also added aruba to make filesystem-tests possible though I didn't add tests which make use of it.

ttaylorr · 2015-01-08T16:38:35Z

lib/geo_pattern/pattern_db.rb

+    public
+
+    def initialize
+      @db = {


@patterns might be a better name, considering it is under an attr_reader.

maxmeyer · 2015-01-08T17:32:09Z

Ok. Fixed.

maxmeyer · 2015-01-08T19:06:14Z

Rebased.

maxmeyer · 2015-01-08T20:01:34Z

Ready to go 🚀

jasonlong · 2015-01-08T20:17:01Z

Thanks @maxmeyer. I'm going to take a little time to get my head around this one. Hopefully I can do so in the next couple of days. Thanks again for all the work here!

maxmeyer · 2015-01-08T20:59:58Z

No problem. You're welcome. Thanks a lot for making this library public. 😃

Hopefully I can do so in the next couple of days.

Would be great! I started using this library in a few projects of mine to generate some small eyecatchers. And spread the word about it. It's quite amazing how beautiful those generated images are.

maxmeyer · 2015-01-11T23:13:19Z

BTW:

I added a few more tests. While writing them I also found a bug in the previous implementation of mine. Which is fixed now. I also added a proper exception for an invalid pattern which will be raised instead of "SystemExit" which is used by abort. Something I would expect to be raised by a commandline application but not a library.

maxmeyer · 2015-01-11T23:14:45Z

lib/geo_pattern/errors.rb

@@ -0,0 +1,7 @@
+module GeoPattern
+  # user errors
+  class UserError < StandardError; end


I think using this kind of pattern for exceptions is a question of style. If you do not like it, I can remove the "intermediate" error.

I don't have an opinion on this. @ttaylorr, do you?

I use this pattern all the time -- looks great.

…place

…pattern because of name conflict

jasonlong · 2015-01-12T19:27:33Z

The changes here seem ok to me. Much of it effects @ttaylorr's recent restructuring changes, so I want to make sure we have a 👍 from him as well.

maxmeyer · 2015-01-12T20:15:11Z

Ok. Thanks for the feedback.

maxmeyer · 2015-01-15T17:46:52Z

@ttaylorr ping :-)

ttaylorr · 2015-01-15T21:41:15Z

Sorry that took so long, it's been a crazy few days for me!

Everything looks fine to me, but I would've implemented a PatternSieve class which has the responsibility of selecting available filters. If we don't want to go that route, this PR is fine to merge as-is.

maxmeyer · 2015-01-19T17:06:20Z

I'm not sure if I understand you correctly. Do you suggest to move this method and those following this to a separate class? I'm open for suggestions. :-)

jasonlong · 2015-01-21T14:28:28Z

Friendly @ttaylorr ping. You aren't busy are you? 😉

maxmeyer · 2015-01-21T16:29:44Z

😄

maxmeyer · 2015-01-24T20:06:37Z

I think I've got an idea what he has got in his mind. @ttaylorr I think we're mixing multiple things in PatternDb:

"Translating" strings to classes
Checking if a pattern is valid
Filtering patterns

Correct @ttaylorr? So I will try to splitup PatternDbinto two or three classes. One for each task mentioned above.

ttaylorr · 2015-01-24T20:10:02Z

Yup. PatternDB has too many responsibilities right now. I think the
PatternDB class is fine to take those first two tasks, but the filtering
of Patterns should be done somewhere else.

What I had suggested earlier (albeit in some fairly ambiguous language) was
to create a PatternSieve which has a PatternDB as reference and a
Filter upon which it can narrow down options in the backing DB. A call
to PatternSieve#valid_patterns should return all elements in the backing
PatternDB #selected with the Filter object.

The Filter class would have one method Filter#is_valid(Pattern) which
would take in a pattern and return true/false depending on whether or not
that pattern matches the criteria set by that filter.

Make sense?

On Sat, Jan 24, 2015 at 3:06 PM, maxmeyer notifications@github.com wrote:

I think I've got an idea what he has got in his mind. @ttaylorr
https://github.com/ttaylorr I think we're mixing multiple things in
PatternDb:

"Translating" strings to classes

Checking if a pattern is valid

Filtering patterns

Correct @ttaylorr https://github.com/ttaylorr? So I will try to splitup
PatternDbinto two or three classes. One for each task mentioned above.

—
Reply to this email directly or view it on GitHub
#40 (comment).

Thanks,
Taylor Blau

ttaylorr.com

maxmeyer · 2015-01-24T20:12:39Z

Yes. I put together some more commits and push them as soon as I can. 😁 I think
is_valid -> valid? is a better choice ;-P.

ttaylorr · 2015-01-24T20:13:26Z

Mixing my Java conventions with my Ruby conventions! Filter#valid? is a
much better choice in Ruby :)

On Sat, Jan 24, 2015 at 3:12 PM, maxmeyer notifications@github.com wrote:

Yes. I put together some more commits and push them as soon as I can. [image:
😁] I think
is_valid -> valid? is a better choice ;-P.

—
Reply to this email directly or view it on GitHub
#40 (comment).

Thanks,
Taylor Blau

ttaylorr.com

maxmeyer · 2015-01-24T20:14:00Z

Maybe I rename PatternDB to PatternTranslator. But only if I also outsource the validation part.

ttaylorr · 2015-01-24T20:25:32Z

I would go with PatternStore, but either will work 😄.

On Sat, Jan 24, 2015 at 3:14 PM, maxmeyer notifications@github.com wrote:

Maybe I rename PatternDB to PatternTranslator. But only if I also
outsource the validation part.

—
Reply to this email directly or view it on GitHub
#40 (comment).

Thanks,
Taylor Blau

ttaylorr.com

maxmeyer · 2015-01-25T06:53:16Z

@ttaylorr
I like the name PatternStore. Changed the implementation already. I think I will push the new implementation this evening.

maxmeyer · 2015-01-25T18:25:36Z

One thing I would like you to reconsider:
Changing the "pattern:"-option one last time from support classes to supporting Symbols by default. I think classes reveal too much implementation details and Symbols are much more common. The new implementaiton will support them as well. I will just change the deprecation warnings.

ttaylorr · 2015-01-25T18:30:41Z

I disagree with it revealing too many implementation details, but for the
sake of Symbols being more common, this seems like a win to me.

On Sun, Jan 25, 2015 at 1:25 PM, maxmeyer notifications@github.com wrote:

One thing I would like you to reconsider:
Changing the "pattern:"-option one last time from support classes to
supporting Symbols by default. I think classes reveal too much
implementation details and Symbols are much more common. The new
implementaiton will support them as well. I will just change the
deprecation warnings.

—
Reply to this email directly or view it on GitHub
#40 (comment).

Thanks,
Taylor Blau

ttaylorr.com

maxmeyer · 2015-01-25T21:18:19Z

I disagree with it revealing too many implementation details

You're revealing the structure of your library in the API by using e.g. GeoPattern::XesPattern. If you want to refactor your library and you need to change the structure of your library you would change the API as well: GeoPattern::XesPattern => GeoPattern::Generators::XesPattern.

The reason I raise this issue is the following: When introducing the Pattern-class (#44) I think it does make sense to rename the XesPattern etc. as well and name them according their function, XesPatternGenerator or wrap them in a Generator-module.

maxmeyer · 2015-01-25T21:23:19Z

lib/geo_pattern/pattern_store.rb

+    public
+
+    def initialize(
+      store = HashStore.new(


This is kind of a HashWithIndifferentAccess. So there's no need to use strings here anymore.

maxmeyer · 2015-01-25T21:26:28Z

lib/geo_pattern/pattern_store.rb

+
+    def [](pattern)
+      if pattern.kind_of?(String) || pattern.kind_of?(Symbol)
+        $stderr.puts 'String pattern references are deprecated as of 1.3.0' if pattern.kind_of?(String)


I added the deprecation warnings for string + class. I can revert this change @jasonlong. It's just a single commit.

maxmeyer · 2015-01-25T21:30:06Z

@jasonlong @ttaylorr What do you think about the new structure?

ttaylorr · 2015-01-26T00:35:45Z

spec/support/helpers.rb

+  #
+  # stream = capture(:stderr) { system('echo error 1>&2') }
+  # stream # => "error\n"
+  def capture(stream)


This is also in Kernel::capture?

@ttaylorr it is, but this method is being deprecated due to it not being thread safe.

You can check it out here https://github.com/rails/rails/blob/346677b3137a35de65c660c230d6fe9e71aa9b0e/activesupport/lib/active_support/core_ext/kernel/reporting.rb

And here is the PR that talks about it rails/rails#13392

Sorry for the random interjection I'm working a similar problem in teachers_pet

maxmeyer · 2015-01-26T05:27:17Z

@ttaylorr Fixed duplication of code. Yes, it's marked deprecated because of missing thread-safety, but I think this should be ok for testing. What do you think?

jasonlong · 2015-01-26T12:56:10Z

Guys, I apologize for my responsiveness. I've been dealing with some personal/family matters lately and haven't been able to focus much on this.

Most of this is lower level Ruby than I'm comfortable with, so I'm happy to defer to @ttaylorr, @tarebyte, and others to collaborate with you (@maxmeyer) on these decisions.

ttaylorr · 2015-01-26T18:41:07Z

👍

maxmeyer · 2015-01-26T18:41:46Z

😄 Love to see this merged.

maxmeyer · 2015-01-26T18:44:32Z

@jasonlong

Guys, I apologize for my responsiveness. I've been dealing with some personal/family matters lately and haven't been able to focus much on this.

Hope everything is fine again for you/your familly soon!

Make it possible to reduce available patterns

jasonlong mentioned this pull request Jan 8, 2015

Add Test Tool and QA Tools #38

Merged

ttaylorr reviewed Jan 8, 2015
View reviewed changes

maxmeyer reviewed Jan 11, 2015
View reviewed changes

maxmeyer added 12 commits January 12, 2015 07:13

Introduce PatternDb to bundle logic for choosing the patterns at one …

76b5a9d

…place

Move code to helpers to make it resuable for testing

07fae49

Use new helpers in tests

8d6d1d0

Added silence helper from active support to reduce noise from tests

051608b

Improve error message

8e3d65c

Add more tests for "bad" path

7856ecb

Document new functionality

c190444

Changed name of variable to make its intend clearer

d49ed0f

A bettern name for interator-variable

3d71659

Make validations more readable

92d611e

Make pattern name to reveal its intention

fb0cca9

Add proper specs for patterndb and fixed a bug: pattern -> requested_…

64b1793

…pattern because of name conflict

maxmeyer added 2 commits January 25, 2015 07:39

Refactored structure

fcc2101

No needed anymore

38870d1

maxmeyer mentioned this pull request Jan 25, 2015

[Proposal] Add Pattern-class #44

Closed

maxmeyer reviewed Jan 25, 2015
View reviewed changes

Prepared to deprecate strings / classes as references

1b3a773

maxmeyer reviewed Jan 25, 2015
View reviewed changes

ttaylorr reviewed Jan 26, 2015
View reviewed changes

Duplicate capture

09db655

maxmeyer mentioned this pull request Jan 26, 2015

Add background color accessor #45

Closed

jasonlong pushed a commit that referenced this pull request Jan 26, 2015

Merge pull request #40 from maxmeyer/feature/reduce_patterns

54497af

Make it possible to reduce available patterns

jasonlong merged commit 54497af into jasonlong:master Jan 26, 2015

Make it possible to reduce available patterns #40

Make it possible to reduce available patterns #40

Conversation

maxmeyer commented Jan 7, 2015

maxmeyer commented Jan 7, 2015

Choose a reason for hiding this comment

maxmeyer commented Jan 8, 2015

maxmeyer commented Jan 8, 2015

maxmeyer commented Jan 8, 2015

jasonlong commented Jan 8, 2015

maxmeyer commented Jan 8, 2015

maxmeyer commented Jan 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonlong commented Jan 12, 2015

maxmeyer commented Jan 12, 2015

maxmeyer commented Jan 15, 2015

ttaylorr commented Jan 15, 2015

maxmeyer commented Jan 19, 2015

jasonlong commented Jan 21, 2015

maxmeyer commented Jan 21, 2015

maxmeyer commented Jan 24, 2015

ttaylorr commented Jan 24, 2015

maxmeyer commented Jan 24, 2015

ttaylorr commented Jan 24, 2015

maxmeyer commented Jan 24, 2015

ttaylorr commented Jan 24, 2015

maxmeyer commented Jan 25, 2015

maxmeyer commented Jan 25, 2015

ttaylorr commented Jan 25, 2015

maxmeyer commented Jan 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmeyer commented Jan 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmeyer commented Jan 26, 2015

jasonlong commented Jan 26, 2015

ttaylorr commented Jan 26, 2015

maxmeyer commented Jan 26, 2015

maxmeyer commented Jan 26, 2015