Support commented out config file #49
Conversation
Modify parser to produce an empty `List[Directive]` if the input is nothing but comments as opposed to failing to parse
| @@ -98,7 +98,6 @@ object ConfigParser { | |||
|
|
|||
| lazy val ident: Parser[Name] = for { | |||
| n <- satisfy(c => Character.isLetter(c)).map2(takeWhile(isCont))(_ +: _) | |||
| _ <- failWhen[Parser](n == "import", s"reserved word ($n) used as an identifier") | |||
There was a problem hiding this comment.
I deleted this line because it doesn't do anything. It's a comparison between an Array and a String so it will never match. Fixing the equality problem is not very difficult (and making it typesafe), but after writing a test for this and trying to get the error to bubble up to the top, I gave up.
In any event, the parser hasn't been restricting this so far and it seems to work...
There was a problem hiding this comment.
I see it from sansImport if we do n.mkString, but since it never worked and wasn't tested, I'm comfortable with dropping it.
|
Ping. Would be really nice to remove this line in quasar. |
| lazy val topLevel: Parser[List[Directive]] = "configuration" |: { | ||
| directives << skipLWS << realEOF | ||
| } | ||
| lazy val topLevel: Parser[List[Directive]] = topLevel(withImport = true) |
There was a problem hiding this comment.
Not a huge fan of shadowing the value name like this. Is there a reason this was done? Seems janky.
There was a problem hiding this comment.
I'm not 100% sure what you mean as I don't see anything being shadowed. Are you saying you are not a fan of having the value topLevel and the definition topLevel share the same name? That kind of thing doesn't seem to bother me all that much, I guess I haven't been sufficiently burned by doing it yet. I guess I did it to avoid using a default value for withImport, not sure why I did that. I'm happy to rename one of the two or give withImport a default value. Do you have a suggestion?
There was a problem hiding this comment.
Renaming nullary topLevel, or adding a defaulted argument to it, would break both source and binary compatibility. So do several of the other changes in this object. Is there a way we can accomplish this without requiring a major version bump?
There was a problem hiding this comment.
It would be possible to separate out the changes that are making the implementation more DRY from the fix to the bug I am trying to address with this PR. The fix to the bug is probably binary compatible (though it would need to be duplicated for both parsers) whereas the refactoring probably needs to break binary compatibility. How import though is it to preserve binary compatibility? I feel like knobs is more of an end-user dependency so it shouldn't matter too much whether this change results in minor or major version bump.
There was a problem hiding this comment.
I think this can be fixed in two vals (sansImport and directives) without any breaking changes. While I agree that version bumps are relatively cheaper in this sort of library, I'd still prefer to avoid churn where it's easy.
|
Both issues fixed in #51. Thanks! |
|
Awesome! Thx! |
Modify parser to produce an empty
List[Directive]if the input is nothing but comments as opposed to failing to parse