YAMLHelper Detecting Merge Conflicts in Podfile #147
Conversation
…th the supplied YAML. Also made distinction on YAMLHelper methods.. i.e. string v file
…load_string and #load_file methods on YAMLHelper
| # @return [Hash, Array] the Ruby YAML representaton | ||
| # | ||
| def load_file(file) | ||
| return load_string(file.read, file.path) |
There was a problem hiding this comment.
Redundant return detected.
Trailing whitespace detected.
There was a problem hiding this comment.
Either the file should be coerced to a Pathname or File.read should be used.
| end | ||
|
|
||
| describe 'Loading Files' do | ||
| it 'raises an Informative error when it encounters a merge conflict' do |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
|
Holy crap that hound is picky. Hound should probably be put on the same page with the Rubocop inspector inside core (in terms of standards)... or the other way around. |
|
@tayhalla we're waiting on Hound's maintainers to add a way for the |
| # Loads a YAML file and leans on the #load_string imp | ||
| # to do error detection. | ||
| # | ||
| # @param A file. |
There was a problem hiding this comment.
The documentation of the parameters should be something along:
@param [#to_s] file
The file to read
|
Houd, although a bit noisy as a solution, appears to be raising good points in this specific patch. Rubocop should identify the same issues. Isn't this the case? Other than cosmetics, this patch looks good. It is missing an entry in the Changelog thought to acknowledge its noble creator. |
Evidently from the build this is not the case. Strange trailing whitespace is one of the main reasons to have RuboCop. |
| else | ||
| raise exception | ||
| raise Exception, "Error parsing YAML: #{yaml_string}" |
There was a problem hiding this comment.
This should be a more specific error class. How about an Informative one like above?
|
@tayhalla Hah, yeah, that hound can sure be picky… Great work, though. Thanks! |
|
Cool - No prob. I'll run back through the patch, take care of Mr. Hound's concerns, update some of the code per your guys' comments, and submit a new commit in a bit here. |
|
Made some changes per @irrationalfab and @alloy. Feel free to check em' out to make sure I'm on the same page with you. Cleaned up some differences between the Hound (http://bit.ly/1ptlXa2) and RuboCop (http://bit.ly/1dtigWe). They seem to have a difference of opinion on single v. double quotes though... They can fight that one out. |
|
👍 |
|
Ace work! |
|
Thanks for the great work @tayhalla |
In #100, the goal was to fix the issue #69, but the fix did not address the issue. As mentioned in #100, the current YAMLHelper parser method - #load - accepts either a file or string, but fails to provide any useful info back to the user about what went wrong, and where. I broke the intake into two methods (as previously discussed). #load_string and #load_file
The PR addresses merge conflicts in the Podfile.lock, telling the user explicitly when it has found them. If it cannot find merge conflicts, it shows the file path (if provided). If only a string is provided, it spills out the offending string to the user.
The previous patch in #100 wasn't doing the trick because that PR assumed only strings were being passed to #load. When it tired to scan the text for any signs of "<<<<head", it was scanning the wrong string, so you were left with the same ambiguous error.
I ran into this because I had merge conflicts in my Podfile.lock, then found all of these past issues. New tests provided, and old ones updated.