Fix duration parsing in lists

[olii]

There are two changes:

• Replace regex with pyparsing objects. Most important is Or() operator that matches the longest match.
• Fix a bug in ListParser.postParse() that ignored some object types.

This fixes: #252

[coveralls]

Coverage increased (+0.8%) to 96.185% when pulling 16a4386 on olii:fix-duration-parsing-in-lists into 07758b8 on chimpler:master.

[scottj97]

Thank you for this.

It's not clear what the first commit is fixing (though I agree it's an improvement). Can you provide a test that was previously failing?

The second commit includes a significant refactoring of the code in addition to the bug fix (which is small), making it hard to spot. (I think it's only adding "isinstance(token, str) and"?)

Can you split that into two commits so the fix is more obvious?

[olii]

I went through the code once again to see if I can split it into multiple commits with tests. The answer is no. A test that is added addresses both bugs.

The first bug is that the regex that we used to match durations was matching the end of the line because there was $ at the end of the regex. This prevented us to correctly match the sequence in a list because such regex will never match (hocon file):

foo: [10 days, bar]

but this was matched OK (hocon file):

foo: 10 days

The problem here was clearly the regex.

Once I fixed this I noticed that the test I wrote doesn't work. The parsed config didn't match the expected value and for some reason, the duration value was omitted from the list. If I put this into the config:

foo: [10 days, bar]

the parsed result was missing a value:

{foo: ['bar']}

This is the second bug I addressed. As you pointed out the trick was to include the type check isinstance(token, str) into the condition. While I was debugging it I had to replace the list comprehension. There are two reasons for it:

• it is hard to debug in the debugger,
• it is hard to understand because there more layers of iterations and conditions involved in the logic.

Once I fixed this the test was passing. I opened a single PR as I believe the bugs are related to each other. I don't think there is a way to write a separate test for each commit.

I reviewed the commits and improved some comments plus I dropped the unneeded import that I moved from a function scope to the module scope (this was an unrelated change).

[scottj97]

I see the issue and I agree about the tests. Thanks for the explanation.

I won't harp on this anymore but I still think the refactoring should be a separate commit from the bug fix.

[olii]

Any update on this, please?

[scottj97]

I created my own branch to explain what I meant about splitting up the refactoring from the bugfix. Commit 195b0e9 does the refactoring, then 588ed9a applies the bugfix.

By splitting them up this way, it clarifies exactly how the fix works.

Sadly, it seems that @chimpler has abandoned this repo.

[olii]

Feel free to close this PR and merge your changes if you have permission to do so.

[darthbear]

Thank you @olii!