search

bigcartel / dugway

Easily build and test Big Cartel themes.
https://developers.bigcartel.com/api/themes
MIT License
149 stars 22 forks source link

New path interpreter #131

Closed jamelablack closed 9 years ago

biscuitvile commented 9 years ago

@jamelablack This is a good encapsulation of a responsibility that didn't need to be in controller.rb You said you were working on some tests for this class so let's wait on this PR until then and then also discuss a bit more. I'm curious how @mattwigham's commit got in there we might want to look at how you made your branch and cleanup the history before a PR. Thanks!

outerim commented 9 years ago

Also shouldn't a class within lib/dugway be inside a Dugway module

jamelablack commented 9 years ago

Thanks @outerim / @biscuitvile! PR updated. Lee, I was curious what you thought about testing the PathInterpreter case statement in the unit/request_spec.rb (https://github.com/jamelablack/dugway/blob/new-PathInterpreter/spec/units/dugway/request_spec.rb#L19-L83) versus elsewhere.

biscuitvile commented 9 years ago

@jamelablack I think it would be good to have the test for the call class include an assertion for each branch of the the case statement instead of the single assertion that it returns a regex.

The actual path interpreter class I might personally change a lot, to avoid using so much regex. I might use String.include? to make some of that more readable, prefer private methods over the constants and extract more private methods. But honestly there could be a lot of logic related to a missing Path domain in Dugway that is looking for a home. There's plenty to clean up and I think for now leaving PathInterpreter as you have it is good enough or perhaps just even best for the time being.

I would merge this PR if you update the test as I've suggested, even if the request spec seems to hit this logic at a higher level. In general, I think it's safe to say a unit test should be more thorough than the high level test that hits its associated code. That way a 'heavier' request spec or acceptance test can be trimmed down a bit and you can still feel safe knowing that the unit test is thorough.

Thanks! I've also added this to Code Climate publicly so if you're looking for more refactor opportunities you can use this to guide you: https://codeclimate.com/github/bigcartel/dugway