New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception handling #844

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vincent-prz

vincent-prz commented Jun 18, 2018

This aims to fix #255

Does this look good?

@eliasdorneles

This comment has been minimized.

Member

eliasdorneles commented Jun 18, 2018

So I've been pairing on this with @Heisenberg815 for some time, I think we're pretty close now.

@Heisenberg815: i was looking at the build failure, it looks like we missed the except (exc1, exc2) case =)
To be continued !

"Warning",
"ZeroDivisionError",
]
if node.type in builtin_exceptions:

This comment has been minimized.

@BPYap

BPYap Jun 26, 2018

Contributor

@Heisenberg815 @eliasdorneles, so I was compiling asyncio modules using the codes from this PR and I encountered a minor bug where voc transpilation fails when node.type is None.

A potential fix would be adding or node.type is None condition on this line (line 2367) to redirect the program flow to self._visitCoreException(node), where node.type is None is being handled.

Cheers 😃

Exception handler: add some fixes.
Handle the case where node.type is None.
Fix check for explicitely named builtin errors.
@vincent-prz

This comment has been minimized.

vincent-prz commented Aug 4, 2018

@BPYap I have made the fix you suggested, plus one other small fix. It seems I still have one test to fix, will do it shortly.

Sorry for the late response.

Cheers!

@eliasdorneles

This is almost good, some minor changes before it's good!

print('Done.')
""")
self.assertCodeExecution("""

This comment has been minimized.

@eliasdorneles

eliasdorneles Sep 7, 2018

Member

Can you please put this test in a separate method, so that it can run as well?

This seems to be a regression (i.e., if i'm not mistaken, this second assertCodeExecution passes on master)

This comment has been minimized.

@vincent-prz

vincent-prz Oct 13, 2018

Done. Indeed, there seems to be a regression on master. But it seems it's the other way around: the second assertCodeExecution passes, while the first fails.

Do you think it's blocking the merge?

This comment has been minimized.

@eliasdorneles

eliasdorneles Oct 15, 2018

Member

Thanks!
Yeah, that's what I said -- the second passes. :)

So, while the patch seems to indeed already make things better, I would like to have a look with more patience to see what's causing that regression.
I'm a bit swamped lately but I'll have some days off soon and will look into it!

This comment has been minimized.

@eliasdorneles

eliasdorneles Oct 15, 2018

Member

I noticed that there are some tests failing related to the time module -- can you please sync w/ master?
Thank you!

@node_visitor
# [FIXME] - make it work for Tuples

This comment has been minimized.

@eliasdorneles

eliasdorneles Sep 7, 2018

Member

leftover to remove?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment