Add some features - #10
Conversation
There was a problem hiding this comment.
Hi, @Paxa. Thank you for finding that project interesting. As you can see I'm not maintaining it at this moment. Actually that was my first gem ever, so you can find some strange decisions I made back there 4 years ago. If you find it usable in your project, just use fork. But for now I can't merge your PR due to some comments down there. If you would like to talk and even finish it to be production ready I'm open for discussion.
Thanks again,
Ivan
| # Specify your gem's dependencies in sinatra-soap.gemspec | ||
| gemspec | ||
|
|
||
| gem 'sinatra', '2.0.0.rc2' |
There was a problem hiding this comment.
I guess this should be moved into gemspec (well it's there).
| rvm: | ||
| - 1.9.3 | ||
| - 2.0.0 | ||
| - 2.4.0 |
There was a problem hiding this comment.
Why would you like to drop supporting for 1.9 branch without major release? Only major release should make breaking API changes
There was a problem hiding this comment.
Probably updating major version will be right. 1.9 support ended 2 years ago, do you know if anyone still using it? I haven't seen it for years
https://www.ruby-lang.org/en/news/2014/01/10/ruby-1-9-3-will-end-on-2015/
| File.join(File.dirname(__FILE__), "..", "views") | ||
| end | ||
|
|
||
| def hash_to_xml(xml, hash) |
There was a problem hiding this comment.
I guess this should be rewritten. Too long method with huge complexity.
There was a problem hiding this comment.
Yep. But what you think about this idea in general? (@.. for xml attributes, @@content to create tag with attributes and simple content, converting array list of singularized tags)
|
|
||
| def call_action_block | ||
| request = Soap::Request.new(env, request, params) | ||
| if defined?(logger) && logger |
There was a problem hiding this comment.
It looks like Soap::Request should know about logging and log request while/before/after execution.
| rescue Soap::Error => e | ||
| builder :error, locals: {e: e}, :views => self.soap_views | ||
| if defined?(logger) && logger | ||
| logger.info "SOAP Request: #{env['HTTP_SOAPACTION']} - Undefined Soap Action" |
There was a problem hiding this comment.
That's not correct. If we catch error that does not mean we catch Undefined Soap Action.
|
|
||
| def get_wsdl | ||
| if defined?(logger) && logger | ||
| logger.info "SOAP: wsdl request" |
There was a problem hiding this comment.
Thats it? seriously? Just log that wsdl request?
There was a problem hiding this comment.
it was useful in my case to verify if client working correctly or not. Probably just standard sinatra's logging will be enough
| def header | ||
| return orig_params[:soap_header] unless orig_params[:soap_header].nil? | ||
| rack_input = env["rack.input"].read | ||
| env["rack.input"].rewind |
There was a problem hiding this comment.
Prefer single quotes (rubocop)
There was a problem hiding this comment.
never understood the point of this rule, afaik now days compiler is smart enough to determine if have interpolations or not...
Will update it when code is production ready
| spec.add_development_dependency "rake" | ||
| spec.add_development_dependency "rack-test" | ||
| spec.add_development_dependency "debugger" | ||
| #spec.add_development_dependency "debugger" |
There was a problem hiding this comment.
Why would you remove debugger out of development dependencies?
There was a problem hiding this comment.
It's only for ruby 1.9.*, I think there was some installation issues with ruby 2.4
Added:
reply_name:param. e.g.soap :action, reply_name: 'ActionReply' do ... endsoap:Headerresponseobject:If you find this useful, I will add more tests and documentation