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

Switch between fake & real fs. #24

Closed
wants to merge 4 commits into from
Closed

Conversation

thsur
Copy link

@thsur thsur commented Aug 12, 2016

Somewhat related to #23 & #7 - I needed to read from the original file system, and this is what I came up with to switch between both file systems.

Background story: I'm testing a command line app lazy-loading subcommands via some loader class that requires the appropriate files. With the switch implemented, mocking the loader was easy as saying:

class Loader

  alias_method :_load, :load

  def load(file)
    MemFs.halt { _load(file) }
  end

end

So this is might be a valid use case, or it might be not, or it might be a stupid implementation, but I thought to give it a try.

# @return nothing
def halt

switch_to = -> dir_class, file_class {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer do...end for multi-line blocks without chaining.
Use the lambda method for multi-line lambdas.
Wrap stabby lambda arguments with parentheses.
Trailing whitespace detected.

Copy link
Author

Choose a reason for hiding this comment

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

Ups, sorry for the many violations - feel free to rewrite the code if you like the feature. I'm still a sort of apprentice when it comes to Ruby...

@simonc
Copy link
Owner

simonc commented Aug 14, 2016

Hi @thsur and thanks a lot for this cool idea!

I think the halt mechanism could be implemented calling deactivate and activate instead of copying the code, that would prevent any error if the activation code changes in the future ☺️

It would also require tests before being merged 😉

@thsur
Copy link
Author

thsur commented Aug 14, 2016

Hi @simonc,

I think the halt mechanism could be implemented calling deactivate and activate instead of copying the code

yep, you're right - I didn't focus enough on my problem, which was not so much switching between file systems, but to keep the state of the faked one.

So I've rewritten the code and also wrote a spec for .halt - since I've no experience with RSpec, you might want to review the latter...

@simonc
Copy link
Owner

simonc commented Aug 18, 2016

Hi @thsur. I think we have several options here:

  1. add a clear: true argument to activate! that way we can disable MemFs and keep the state
  2. Add a real API to FileSystem to dump/restore the current state

I would vote for the :clear option, it's the simplest one.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage increased (+0.001%) to 99.831% when pulling 9cf83ef on pontycode:switch-fs into 90607ac on simonc:master.

@thsur
Copy link
Author

thsur commented Aug 25, 2016

Hi @simonc,

sorry, took me a while to come back to the issue. I wanted to introduce some sort of parameter for activate! myself, but didn't want to clutter your method signatures. But you're right, it's the easiest one, so it's in.

before :each do
described_class.activate!
described_class.deactivate!
end
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of this before block?

@simonc
Copy link
Owner

simonc commented Aug 25, 2016

Hey @thsur. That's pretty cool. Two small things though:

  1. the comment regarding the before block
  2. I think we should have a spec regarding the ensure that tests that halt re-actives MemFs in case an exception happens

After this, it's a GO! :shipit:

@coveralls
Copy link

coveralls commented Aug 26, 2016

Coverage Status

Coverage increased (+0.001%) to 99.831% when pulling 4ebbed4 on pontycode:switch-fs into 90607ac on simonc:master.

@thsur
Copy link
Author

thsur commented Aug 26, 2016

Hey @simonc, thanks for your comments!

What is the purpose of this before block?

Hm. Good question. Maybe it's purpose was to illustrate my lack of talent when coding by copy & paste...

I think we should have a spec regarding the ensure that tests that halt re-actives MemFs in case an exception happens

Absolutely. It's in now.

@simonc simonc closed this in 722acb4 Aug 29, 2016
@simonc
Copy link
Owner

simonc commented Aug 29, 2016

Merged! 🎉
Thanks a lot! ☺️

@thsur
Copy link
Author

thsur commented Aug 30, 2016

Thank you! 🍻

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.

4 participants