Nokorigi is always required

[prodis]

Nokogiri gem is required even if handler is configured with Ox.

[krasnoukhov]

At the moment nokogiri is required only if ox can't be loaded:

sax-machine/lib/sax-machine.rb

Lines 17 to 21 in 20d35ac

	begin
	SAXMachine.handler = :ox
	rescue LoadError
	SAXMachine.handler = :nokogiri
	end

Am I missing something here?

[prodis]

I tried to use SAXMachine gem without install Nokogiri, only with Ox gem installed and I got a Nokogiri load error. This happens because lib/sax-machine/sax_document.rb file requires Nokogiri on top, in order to use it in parse method.

sax-machine/lib/sax-machine/sax_document.rb

Lines 1 to 29 in 20d35ac

	require "nokogiri"
	module SAXMachine
	def self.included(base)
	base.send(:include, InstanceMethods)
	base.extend(ClassMethods)
	end
	def parse(xml_text, on_error = nil, on_warning = nil)
	if SAXMachine.handler == :ox
	Ox.sax_parse(
	SAXOxHandler.new(self, on_error, on_warning),
	StringIO.new(xml_text),
	{
	symbolize: false,
	convert_special: true,
	skip: :skip_return,
	}
	)
	else
	handler = SAXNokogiriHandler.new(self, on_error, on_warning)
	parser = Nokogiri::XML::SAX::Parser.new(handler)
	parser.parse(xml_text) do |ctx|
	ctx.replace_entities = true
	end
	end
	self
	end

[krasnoukhov]

Damn, you're right. I've removed this reference which was mistakenly left 🙅 Can you now try the master please?

[prodis]

Now, we can't instance Nokogiri::XML::SAX::Parser class in parse method.
https://github.com/pauldix/sax-machine/blob/master/lib/sax-machine/sax_document.rb#L20

How about my pull request #55?
I eliminated the if statement in parse method, created a factory class and extracted parser logic to handler manager classes. Only these classes require specific handler.
This way, if you need to use another handler in future, besides Nokogiri and Ox, you just need to create one or two specific handler classes, instead of to write a else or switch case statement in parse method.

[krasnoukhov]

Please consider following gist. As you can see, everything works fine and instance of Nokogiri::XML::SAX::Parser is created just fine. Another thing is that ox is not required on top of sax_document.rb, but Ox.sax_parse is called without any issue. This works because when you require something in Ruby, loaded modules/classes/constants are added to object space for a time of program execution, and we require at least one handler before sax_document.rb. Also, please note that tests are passing.

Regarding the #55, I think it's now clear that it does not actually solve the given problem. I see that your intention was to get rid from if or case statement with factory pattern. I personally must disagree with this approach. Ruby is not Java, and this case we're dealing with is very simple. We need to switch between handlers in only one place in code so using heavyweight abstraction seems like a bit an overhead for me. Please take a look at my attempt to add another handler option. As you can see, case statement still works for this case and also looks good and simple.

Please let me know what you think. Cheers 💫

[prodis]

About parsemethod you are right, Nokogiri is required on handler class, that is loaded when we choose the handler. My bad.

About the pull request #55, the point is exactly the obligation to modify the parse method implementation for each handler. This procedural approach goes against Open Closed Principle, besides the responsibility accumulation on SAXMachine class, where it knows specific parser logic of each handler. We don't need to use Java to apply object oriented design, we can do it in Ruby, after all everything is an object. But, your choice.

Can you release another version with the current master, please?

[krasnoukhov]

Your point is perfectly valid, so I went ahead and changed parse method to be unaware of handler logic.

[krasnoukhov]

1.0.3 is released. Please give it a try!

[prodis]

Thanks!