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

Moving code to separate classes #245

Closed
wants to merge 24 commits into from
Closed

Moving code to separate classes #245

wants to merge 24 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2015

This is the first PR for #231. I kept the commits as isolated as possible, to make it easier to review the changes.

This PR is not yet finished. I think it needs to be discussed first. All "old" tests are passing with this code. It also duplicates the capture-method I used in some other PR. I only added it to make the tests pass. The documentation is not complete. Tests needs to be written.

Besides that I'm quite happy with the code:

  • It support's the old API and adds a lot of deprecation warnings
  • Now all announcer code is bundled and has a nice API which be extended easily by us and by users - see a very first version of an example in lib/aruba/announcer.rb
  • The announce_or_puts-method is gone and replaced by mode=
  • I introduced a default_announcer which writes to /dev/null and an announcer which writes to what ever announcer was chosen by mode= - e.g. Kernel.puts, puts
  • At first I thought it might be a good idea to store the activated announcers in a Hash, but decided against it because we would end up with different activated Announcers if the mode is changed between two activations
  • All process monitoring code is bundled in ProcessMonitor - BTW I'm open to rename if one of you guys has a better idea.

@jarl-dk @mattwynne

Suggestions?

@jarl-dk
Copy link
Member

jarl-dk commented May 13, 2015

I don't see the relation to the topics mentioned in #231. But never mind.

Regarding this PR: It seems like you have seen some light that haven't reached me yet and it will take some time for me to get there... I will just say that at first glance it all seems ok, for the rest of it I'll just trust you..

@ghost
Copy link
Author

ghost commented Jun 2, 2015

Merged into #257

@ghost ghost closed this Jun 2, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants