Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transformations #14

Merged
merged 3 commits into from
Feb 9, 2015
Merged

Transformations #14

merged 3 commits into from
Feb 9, 2015

Conversation

enriikke
Copy link

  • Finally added the document transformation code!!
  • Updated SkoogleDocs::Document class to use the new SkoogleDocs::Transformer
  • Disabled RSpec monkey patching
  • Included tests and documentation

@coveralls
Copy link

Coverage Status

Coverage increased (+0.65%) to 95.85% when pulling 2ded516 on transformations into 7979687 on master.

#
# @return [SkoogleDocs::Transformer]
def transformer
SkoogleDocs::Transformer.new(@source)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, I can't overwrite? What about using mocks in tests?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ruprict, here. Maybe we could change this declaration to

def transformer(klass = SkoogleDocs::Transformer)
  klass.new(@source)
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thank you both for giving me input on this. I'll change the code to reflect this.
@chuckpreslar is there a difference between defaulting to an actual instance of SkoogleDocs::Transformer instead of the class?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the class allows for a smaller duck-typed interface the class must implement (only new here) and more control to allow the transformer method to function as a factory.

for a hypothetical example, let's say we pass in instances. if the instance we pass it has already been configured with default values, the ability for transformer to manipulate the instance without potentially breaking things is limited.

@ruprict
Copy link

ruprict commented Jan 29, 2015

Looks really good. I especially like the use of FactoryGirl for the HTML docs. Just had one very minor comment, otherwise :shipit:

@chuckpreslar
Copy link

very nice, @enriikke

@coveralls
Copy link

Coverage Status

Coverage increased (+3.2%) to 98.4% when pulling f6e0d25 on transformations into 7979687 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.65%) to 95.85% when pulling f6e0d25 on transformations into 7979687 on master.

enriikke pushed a commit that referenced this pull request Feb 9, 2015
@enriikke enriikke merged commit 7fa5e3e into master Feb 9, 2015
@enriikke enriikke deleted the transformations branch February 9, 2015 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants