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

Make rails dependency an optional #9

Open
3 tasks
JelF opened this issue Jan 2, 2017 · 5 comments
Open
3 tasks

Make rails dependency an optional #9

JelF opened this issue Jan 2, 2017 · 5 comments

Comments

@JelF
Copy link
Contributor

JelF commented Jan 2, 2017

Idea:

  • Exception_notifier somehow could be configured to use appropriate mailer. As far as i see, there is no generalized way to provide secitions views to a generic mailer, but i don't sure exception_notifier not locked to ActionMailer descendants, so there probably could be a way to configure all ActionMailers to respec our views. If not, views should be moved to appropriate generators of frameworks
  • Load rails-related code via special file lamian/rails.rb, so they could be easily loaded, but only if rails requested
  • Probably add hanami/rb / sinatra.rb files for thoose frameworks

@fiscal-cliff offered some help, would you do all this work for me?

@JelF
Copy link
Contributor Author

JelF commented Jan 2, 2017

@fiscal-cliff, I can do #8 to simplify this task for you

JelF added a commit that referenced this issue Jan 2, 2017
1.0.0a would not be launched immediately, but version should be bumped
1.0.0a would be affected by big back-incompatible changes in #9, so it
would be a good time to increment majorest version. Hovewer, i'll
increment it now to simplify Changelog usage.
@fiscal-cliff
Copy link
Contributor

fiscal-cliff commented Jan 15, 2017

I managed to sort out this issue
First of all, the notifier itself has rails as a dependency. The only way to evade this requirement is to use simple_email_exception_notifier which we use already. The notifier provides plain text formatter which is not compatible with :email notifier. So rendering request_log section inside the notifier turned out to be way harder than I expected. I've patched the notifier as follows

module SimpleNotifierPatch
  def compose_message(exception, env, options)
    SimpleEmailExceptionNotifier::Message.compose do |m|
      m.print_summary(exception)
      m.print_section('Backtrace', exception.backtrace)

      m.print_section('Request log', Lamian.logger.dump.split("\n")) # <<< !!!

      unless env.empty?
        m.print_request(Rack::Request.new(env)) if defined?(Rack::Request)
        m.print_section('Environment', filter_env(env))
      end

      m.print_section('Data', extract_data(env, options))
    end
  end
end

ExceptionNotifier::SimpleEmailNotifier.prepend(SimpleNotifierPatch)

I would advice you to look up alternatives for ExceptionNotifier since this solution looks extremely dirty.

@JelF
Copy link
Contributor Author

JelF commented Jan 15, 2017

@fiscal-cliff

I would advice you to look up alternatives for ExceptionNotifier since this solution looks extremely dirty.

ExceptionNotifier-related stuff could be moved to rails-only section in this case, same as it's views and rails engine.
Also, consider use Lamian.dump instead of Lamian.logger.dump, because Lamian.logger is part of private api and this comands do same stuff

@fiscal-cliff
Copy link
Contributor

fiscal-cliff commented Jan 15, 2017 via email

JelF added a commit that referenced this issue Jun 24, 2017
* prepared version dump to next release

1.0.0a would not be launched immediately, but version should be bumped
1.0.0a would be affected by big back-incompatible changes in #9, so it
would be a good time to increment majorest version. Hovewer, i'll
increment it now to simplify Changelog usage.

* Added Changelog.md

* added doc:coverage task into rake default to check yard coverage

* done

* fixed bugs and updated json dependency for ruby 2.4 compatibility

* update travis build matrix to actual supported versions

* updated rubocop to 0.49
@JelF
Copy link
Contributor Author

JelF commented Jul 30, 2017

I will not add non-rails support untill i need it in production. However, pull requests are welcome

Core functionality is rails-independent, same is middleware itself. Rails engine should be required in "lamian/rails" and readme should be updated. Exception notifier API is not required in non-rails for oblivious reasons (exception_notifier bound to rails), so i suggest to add API for Raven

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants