Manage xcdatamodeld #53
Conversation
| Gyro::Log.fail!('Unable to find contents of xcdatamodel', stacktrace: true) unless File.exist?(contents_file) | ||
| is_xcdatamodeld = xcdatamodel_dir.extname != '.xcdatamodeld' | ||
| Gyro::Log.fail!('Please target an xcdatamodel inside your xcdatamodeld') unless is_xcdatamodeld | ||
| if xcdatamodel_dir.to_path.include?('.xcdatamodeld') |
There was a problem hiding this comment.
Change this : xcdatamodel_dir.parent.extname == '.xcdatamodeld
| contents_file = File.join(xcdatamodel_dir, 'contents') | ||
| Gyro::Log.fail!('Unable to find contents of xcdatamodel', stacktrace: true) unless File.exist?(contents_file) | ||
| is_xcdatamodeld = xcdatamodel_dir.extname != '.xcdatamodeld' | ||
| Gyro::Log.fail!('Please target an xcdatamodel inside your xcdatamodeld') unless is_xcdatamodeld |
There was a problem hiding this comment.
wording nitpicking: "Please target an '.xcdatamodel' file inside your xcdatamodeld"
There was a problem hiding this comment.
ok thanks for this
| if xcdatamodel_dir.parent.extname == '.xcdatamodeld' | ||
| xcdatamodeld_info_message = 'You are using an xcdatamodeld, ' \ | ||
| 'please be sure you target the correct version of your xcdatamodel.' \ | ||
| ' Actual version using by gyro is : ' + xcdatamodel_dir.basename.to_path |
There was a problem hiding this comment.
'Current version used by gyro is: ' + xcdatamodel_dir.basename.to_path
There was a problem hiding this comment.
Yes Current is better than actual
There was a problem hiding this comment.
And in english there's no space before :
| document_xml = Document.new(file) | ||
| file.close | ||
| load_entities(document_xml) | ||
| xml_document = Document.new(file_contents.open) |
There was a problem hiding this comment.
Also, maybe XML::Document can take a Pathname? Worth checking
There was a problem hiding this comment.
I did not find any information about REXML::Document with Pathname, the better solution is to rollback my changes.
spec/gyro/xcdatamodel_spec.rb
Outdated
| expect { Parser::XCDataModel::XCDataModel.new(xcdatamodel_dir) } | ||
| .to raise_error 'The relationship "user" from "FidelityCard" is wrong - please fix it' |
There was a problem hiding this comment.
The new spec is good, but why did the old "check raising relationship error" spec disappear?
There was a problem hiding this comment.
My bad I will fix this
| load_entities(xml_document) | ||
| file = File.open(file_contents) | ||
| document_xml = Document.new(file) | ||
| file.close |
There was a problem hiding this comment.
File.open can take a trailing closure/block which will open the file, execute the block with the file handle, then auto-close the file.
document_xml = File.open(file_contents) { |file| Document.new(file) }That's more readable, and also safer as, in case there's an exception on the lines of code before open and close, the block syntax will catch the exception and close the file properly before rethrowing, instead of letting the file open.
| @@ -37,18 +37,20 @@ class XCDataModel | |||
|
|
|||
| def initialize(xcdatamodel_dir) | |||
| is_xcdatamodeld = xcdatamodel_dir.extname != '.xcdatamodeld' | |||
| Gyro::Log.fail!('Please target an xcdatamodel inside your xcdatamodeld') unless is_xcdatamodeld | |||
| Gyro::Log.fail!('Please target an \'.xcdatamodel\' file inside your xcdatamodeld') unless is_xcdatamodeld | |||
There was a problem hiding this comment.
I'm surprised rubocop doesn't suggest you here to use %q(Please target an '.xcdatamodel' file …) syntax here (%q(…) allowing you to avoid having to escape the single quotes in the string)
There was a problem hiding this comment.
ok so I tried with %qbut rubocop told that this syntax is only for string that contain both single quotes and double quotes. I seached and I found %() and %Q().
There was a problem hiding this comment.
And rubocop tells me to use %() instead of %Q()
spec/gyro/xcdatamodel_spec.rb
Outdated
| @@ -27,7 +27,13 @@ module Gyro | |||
| it 'check raise an error for xcdatamodeld' do | |||
| xcdatamodel_dir = fixture('xcdatamodel', 'Model.xcdatamodeld') | |||
| expect { Parser::XCDataModel::XCDataModel.new(xcdatamodel_dir) } | |||
| .to raise_error 'Please target an xcdatamodel inside your xcdatamodeld' | |||
| .to raise_error 'Please target an \'.xcdatamodel\' file inside your xcdatamodeld' | |||
This PR fixes #26.