Fixes a couple of errors in the parser handling of function definition

[MatthieuDartiailh]

One mistake was reported in #251, I found the other while debugging. @frmdstryr feel free to give a look and if you have an idea on how we could test this in clean way, I am very much interested.

[codecov-io]

Codecov Report

Merging #252 into master will increase coverage by 0.6%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #252     +/-   ##
=========================================
+ Coverage   63.32%   63.93%   +0.6%     
=========================================
  Files         288      287      -1     
  Lines       23392    23356     -36     
=========================================
+ Hits        14814    14932    +118     
+ Misses       8578     8424    -154

Impacted Files	Coverage Δ
enaml/core/parser/base_parser.py	71.95% <ø> (+2.33%)	⬆️
enaml/core/parser/parser3.py	76.23% <100%> (+38.5%)	⬆️
enaml/core/parser/__init__.py
enaml/core/parser/base_lexer.py	88.45% <0%> (+0.84%)	⬆️
enaml/core/parser/parser2.py	43.67% <0%> (+5.69%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e185cd9...e75c88e. Read the comment docs.

[gabrielcnr]

@MatthieuDartiailh I wrote a test for this which is probably closer to an integration test rather than a unit test because it tests the parser from a functional POV used with the compiler instead of making punctual assertions against AST types.

What do you think about the below:

from enaml.core.parser import parse, write_tables
from enaml.core.enaml_compiler import EnamlCompiler
from future.utils import exec_


def test_p_varargslist18():
    write_tables()

    source = """
def func(foo=1, bar=2, **kwargs):
    d = {'foo': foo, 'bar': bar}
    d.update(kwargs)
    return d
"""

    ast = parse(source, filename='foo.enaml')
    code = EnamlCompiler.compile(ast, 'foo.enaml')

    ns = {}
    exec_(code, ns)

    func = ns['func']

    assert {'foo': 1, 'bar': 2} == func()

    assert {'foo': 100, 'bar': 200, 'spam': 300} == func(100, 200, spam=300)

If a test in these lines is acceptable I'd be happy to refactor this in a way that makes it easier for writing individual tests for each one of the parsing rules and then contribute by writing tests for all the cases.

[MatthieuDartiailh]

Thanks for your interest @gabrielcnr. Actually reading your proposed test gave me an idea for something that is believe is more robust and easier to automatize (based on ast analysis). I found a bunch of other issues running the tests actually. If you could take the time to read through the tests to check that I have not forgotten anything it would be great.

[frmdstryr]

Overall I'd say it looks good. Is there a reason it's in a separate package? Took me a little to find it there...

The codecov report shows a few cases that are missing https://codecov.io/gh/nucleic/enaml/tree/e75c88e51ddd06daa0b31fde4dafeef503b22d1f/enaml/core/parser

[frmdstryr]

I added a few more cases here: https://github.com/MatthieuDartiailh/enaml/compare/parser_fixes...frmdstryr:parser_fixes?expand=1 but it only increases coverage by about 20 lines.

The last two statements don't pass the ast check so IDK what's going on there.

[MatthieuDartiailh]

Thanks @frmdstryr. I put the tests in a different package because I usually try to follow the files organization for the tests organizations. So far we have little enough tests that it does not matter, but it is worth starting to clean up a bit.
I was also not planning to add total coverage of the parser in this PR (simply of the function definitions). If you are interested in adding more tests, feel free to open another pull request. I will merge this as is and look into the two cases that fail for you later.