Permalink
Browse files

Minor syntactical changes

  • Loading branch information...
1 parent 900432e commit 2a79ad93874b687736edf71e4948280dd6d91aee @mjackson committed Feb 22, 2012
Showing with 9 additions and 10 deletions.
  1. +9 −10 lib/citrus.rb
View
@@ -382,26 +382,25 @@ def self.extend_object(obj)
super
end
- # Parses the given +string+ using this grammar's root rule. Accepts the same
+ # Parses the given +source+ using this grammar's root rule. Accepts the same
# +options+ as Rule#parse, plus the following:
#
# root:: The name of the root rule to start parsing at. Defaults to this
# grammar's #root.
- def parse(string, options={})
+ def parse(source, options={})
rule_name = options.delete(:root) || root
raise Error, "No root rule specified" unless rule_name
rule = rule(rule_name)
raise Error, "No rule named \"#{rule_name}\"" unless rule
- rule.parse(string, options)
+ rule.parse(source, options)
end
- # Parse the given file at +path+ using this grammar's root role. Accept the same
- # +options+ as #parser.
- def parse_file(path, options = {})
- unless path.respond_to?(:to_path)
- require 'pathname'
- path = Pathname.new(path)
- end
+ # Parses the contents of the file at the given +path+ using this grammar's
+ # #root rule. Accepts the same +options+ as #parse.
+ def parse_file(path, options={})
+ path = Pathname.new(path.to_str) unless Pathname === path
+ raise Error, "Cannot find file #{path}" unless path.exist?
+ raise Error, "Cannot read file #{path}" unless path.readable?
blambeau
blambeau Feb 22, 2012 Collaborator

Well, I know at least one gem (epath) that provide a Path implementation, far better than Pathname, and which will not be usable here. Checking for respond_to?(:to_path) is standard in the ruby 1.9.x branch.

Also, I'm not sure that raising a Citrus::Error is actually a good idea if something goes wrong with the file. I would say that the path must exit and be readable as a precondition and that a IOError may occur otherwise.

What do you think?

mjackson
mjackson Feb 22, 2012 Owner

I changed the respond_to?(:to_path) to preserve compatibility with 1.8. I guess that in the worst case if the user does use a Pathname object here under 1.8 and we're doing a respond_to?(:to_path), the only thing that will happen is a Pathname.new(Pathname.new('some/path')), which will just return a Pathname anyway.

I put the error behavior in there to mimic the behavior of Citrus.load, as it raises a LoadError when the file either doesn't exist or is not readable. It's not particularly important to me that we do these checks here, but if we're going to do it one place we should do it in both, right?

blambeau
blambeau Feb 23, 2012 Collaborator

For :to_path, sorry I was a bit confused. We have the two following invariants now, which is good, right?

# The initial parsed source, being a Pathname/File/IO/String is 
# always preserved when invoking #parse
grammar.parse(anything).source == anything

# The source is always a Pathname when invoking #parse_file.
# +anything+ mut respond to :to_str, the later denoting the path to
# an existing file.
Pathname === grammar.parse_file(anything).source

You're right about consistency of error management, so I'm fine with that too. However, for the next major version, it would be better IMO to let IO errors unchanged everywhere (including in Citrus.load, that is). There is nothing Citrus can do if a file does not exists. Hiding the real cause (Errno::ENOENT) behind a Citrus::Error actually prevents the caller from doing something smart (friendly user message, catching the cause and finding a countermeasure, etc.).

mjackson
mjackson via email Feb 24, 2012 Owner
parse(path, options)
end

0 comments on commit 2a79ad9

Please sign in to comment.