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.

Sign up for GitHub

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

Jump to bottom

PyPy support #78

Merged
merged 4 commits into from
Mar 23, 2021
Merged

PyPy support #78

merged 4 commits into from
Mar 23, 2021

Conversation

Stranger6667
Copy link
Contributor

Resolves #77

@Stranger6667 Stranger6667 force-pushed the dd/pypy-support branch 2 times, most recently from c3c93cb to 16feefc Compare March 1, 2021 12:24
@Stranger6667
Copy link
Contributor Author

Not sure what would be the best way to handle mypy errors though :(

@Zac-HD
Copy link
Member

Zac-HD commented Mar 1, 2021

Hmm. Could we just use the PyPy-compatible version unconditionally? Otherwise I'd try to work typing.TYPE_CHECKING into the conditions somehow.

@Stranger6667 Stranger6667 mentioned this pull request Mar 1, 2021
Closed
@Stranger6667
Copy link
Contributor Author

Could we just use the PyPy-compatible version unconditionally?

It seems like there should still be a condition as for the CPython version we need to call _make_iterencode. However, the condition could be moved inside the class itself.

@Stranger6667 Stranger6667 force-pushed the dd/pypy-support branch 2 times, most recently from 1ab0e74 to 23e4409 Compare March 1, 2021 12:54
@Stranger6667
Copy link
Contributor Author

So, the in-class condition worked :) Also, I moved the import together with the type: ignore comment and everything seems to be fine :)

@Stranger6667 Stranger6667 marked this pull request as ready for review March 1, 2021 12:56
@Stranger6667
Copy link
Contributor Author

Introducing these implementation-specific branches affects the code coverage requirements. At the moment, I see these options to fixing the CI:

  • On CI: Run tests without the coverage check, gather coverage from multiple jobs, merge it, and add a dependent job that checks the required coverage level. Having something like this locally will require running tests on PyPy as well, but certainly doable in tox.ini;
  • Skip these branches from coverage :(

Am I missing something? What do you think about this?

@Zac-HD
Copy link
Member

Zac-HD commented Mar 5, 2021

I'm happy to just # pragma: no cover the explicitly-PyPy-only branches, and disable coverage measurement for PyPy jobs if that's the easiest path forwards.

I am a little concerned by the uncovered code in _canonicalise.py under CPython though, and the apparent trace function issue under PyPy is weird (but if the tests all pass I don't mind only checking coverage on CPython).

@Stranger6667 Stranger6667 force-pushed the dd/pypy-support branch 2 times, most recently from 05e19c6 to cb7a1f2 Compare March 5, 2021 16:27
@Stranger6667
Copy link
Contributor Author

I added a couple of changes :)

Re: coverage changes. Interestingly, these lines (763->764, 764-765) in _canonicalise.py become uncovered even if I just move the floatstr function out of the iterencode body. Also, the job duration increased significantly, but I am not sure whether it is something related to randomness or moving this function. It seems unlikely that passing a global function may cause such a difference.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 21, 2021

I'm pretty sure that this failure in PyPy36 is a real failure of canonicalisation rather than a pypy-specific failure; I'll dig in and confirm with either a fix or a workaround later this week 😄

Failed validating 'oneOf' in schema['properties']['requireDotNotation']:
    {'description': 'Requires member expressions to use dot notation when '
                    'possible.',
     'oneOf': [{'type': 'boolean'},
               {'const': None},
               {'description': 'An object that contains an "allExcept" key '
                               'equal to an array of exception values.',
                'properties': {'allExcept': {'description': 'Array of '
                                                            'quoted '
                                                            'keywords to '
                                                            'exempt.',
                                             'items': {'oneOf': [{'description': 'Allow '
                                                                                 'quoted '
                                                                                 'identifiers '
                                                                                 'made '
                                                                                 'of '
                                                                                 'reserved '
                                                                                 'words.',
                                                                  'enum': ['keywords']},
                                                                 {'description': 'Allow '
                                                                                 'quoted '
                                                                                 'snake '
                                                                                 'cased '
                                                                                 'identifiers.',
                                                                  'enum': ['snake_case']}],
                                                       'type': 'string'},
                                             'minItems': 0,
                                             'type': 'array',
                                             'uniqueItems': True}},
                'type': 'object'},
               {'description': 'Deprecated in favor of the object '
                               '"allExcept": ["snake_case"] rule format.',
                'oneOf': [{'description': 'Allow quoted snake cased '
                                          'identifiers.',
                           'enum': ['except_snake_case']}],
                'type': 'string'}],
     'type': ['boolean', 'null', 'object', 'string']}

On instance['requireDotNotation']:
    'except_snake_case'
---------------------------------- Hypothesis ----------------------------------
Falsifying example: test_can_generate_for_real_large_schema(
    data=data(...), name='JSCS configuration file',
)
JSCS configuration file
Draw 1: {'requireDotNotation': 'except_snake_case'}

Which means that pypy support is almost here! Thanks @Stranger6667!

@Stranger6667
Copy link
Contributor Author

Cool! Thank you @Zac-HD :)

@Zac-HD Zac-HD merged commit dd812fa into python-jsonschema:master Mar 23, 2021
@Stranger6667 Stranger6667 mentioned this pull request Mar 25, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zac-HD Zac-HD Zac-HD left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PyPy support
2 participants
@Stranger6667 @Zac-HD