Skip to content
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

Add exception and warning constants #121

Merged
merged 12 commits into from
Nov 17, 2020

Conversation

hodgestar
Copy link
Contributor

@hodgestar hodgestar commented Nov 11, 2020

This adds all the exceptions and warnings from Python's pyerrors.h, except:

  • EnvironmentError (an alias for OSError)
  • IOError (as alias for OSError)
  • WindowsError (only defined on Windows and just an alias for OSError)

The EnvironmentError and IOError aliases are tested from Python code.

It also adds:

  • HPyErr_SetObject (for raising the created unicode exception instances)

It not longer adds HPy_Call and marks the unicode exception tests as expected failures until HPy_Call or similar exists.

@TeamSpen210
Copy link

According to the docs, WindowsError is just a compatibility alias for OSError, so it can probably just be omitted. Maybe IOError and EnvironmentError can be omitted as well with an explanatory comment.

@@ -140,6 +208,8 @@ int HPy_RichCompareBool(HPyContext ctx, HPy v, HPy w, int op);

HPy_hash_t HPy_Hash(HPyContext ctx, HPy obj);

HPy HPy_Call(HPyContext ctx, HPy callable, HPy args, HPy kwargs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is worth being discussed (and probably this is not the correct place, we should open a separate issue): what signature do we want for HPy_Call?
I don't think we want to force the user to create a tuple: I'm thinking of something along the lines of Py_VectorCall in which you pass an array of HPy and the number of arguments, which incidentally also matches the signature for our functions.
I think it's better to remove this from this PR (and deleting/skipping the UnicodeException tests, if they can't be implemented in any other way)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it should be discussed. I like the idea of a signature a bit like our KEYWORDS method signature -- i.e. *args, nargs, kw.

Happy to remove from here and start a new PR with what we decide -- perhaps I will just disable the test for the moment and we can enable it when Py_Call arrives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HPy_Call being discussed in #122.

@hodgestar hodgestar changed the title WIP: add exception and warning constants Add exception and warning constants Nov 12, 2020
@hodgestar
Copy link
Contributor Author

@antocuni HPy_Call removed.
@TeamSpen210 EnvironmentError and IOError removed -- thanks for the suggestion!

Copy link
Collaborator

@antocuni antocuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart the xfail vs skip

@@ -200,6 +200,10 @@ def check_exception(cls):

def test_h_unicode_exceptions(self):
import pytest
pytest.xfail(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pypy's apptest don't support xfail. Can we just skip it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with the pytest.mark.xfail in test_importing.py?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a completely different thing, in the best pypy tradition 🤦‍♂️
pytest.mark.* are handled by pytest at collection time, so xfail works properly.
However, once you are inside the test it's a different story: since it's an apptest, the pytest module which is imported inside the test is a dummy one which implements only skip and raises: see pypy/tool/pytest/objspace.py:maketestobjspace() to see where the (dark) magic happens.

So to summarize: I withdraw my suggestion above, you can just use @pytest.mark.xfail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot. @pytest.mark.xfail used instead.

@hodgestar hodgestar merged commit 62f48e1 into master Nov 17, 2020
@hodgestar hodgestar deleted the feature/add-exception-and-warning-constants branch December 18, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants