Skip to content
This repository has been archived by the owner. It is now read-only.

Fix exception handling #844

Closed
wants to merge 10 commits into from

Conversation

@vincent-prz
Copy link

@vincent-prz vincent-prz commented Jun 18, 2018

This aims to fix #255

Does this look good?

@eliasdorneles
Copy link
Collaborator

@eliasdorneles 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:
Copy link
Contributor

@BPYap BPYap Jun 26, 2018

@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 😃

Handle the case where node.type is None.
Fix check for explicitely named builtin errors.
@vincent-prz
Copy link
Author

@vincent-prz 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!

Copy link
Collaborator

@eliasdorneles eliasdorneles left a comment

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

print('Done.')
""")

self.assertCodeExecution("""
Copy link
Collaborator

@eliasdorneles eliasdorneles Sep 7, 2018

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)

Copy link
Author

@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?

Copy link
Collaborator

@eliasdorneles eliasdorneles Oct 15, 2018

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!

Copy link
Collaborator

@eliasdorneles eliasdorneles Oct 15, 2018

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
Copy link
Collaborator

@eliasdorneles eliasdorneles Sep 7, 2018

leftover to remove?

@phildini
Copy link
Member

@phildini phildini commented Apr 25, 2020

Hi there! It looks like this PR might be dead, so we're closing it for now. Feel free to re-open it if you'd like to continue, or think about directing your efforts to https://github.com/beeware/briefcase or https://github.com/beeware/toga. Both of these have more active development right now. 😄

@phildini phildini closed this Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants